Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revise port matching logic #596

Merged
merged 18 commits into from
May 3, 2023
Merged

Revise port matching logic #596

merged 18 commits into from
May 3, 2023

Conversation

arichiv
Copy link
Member

@arichiv arichiv commented Apr 11, 2023

We need to ensure that:

  1. We are consistently using strings
  2. We handle cases where the default port isn't set
  3. We handle the case where both ports are unset

closes #591

@arichiv
Revise port matching logic
ce571d9 
We need to ensure that:
1. We are consistently using strings
2. We handle cases where the default port isn't set
3. We handle the case where both ports are unset

closes #591
@arichiv arichiv requested a review from annevk April 11, 2023 13:22
@arichiv arichiv self-assigned this Apr 11, 2023
@arichiv
Copy link
Member Author

arichiv commented Apr 19, 2023

@annevk have time to take a look?

annevk
annevk reviewed Apr 20, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Apr 20, 2023
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 20, 2023
@arichiv @chromium-wpt-export-bot
[CSP] WPTs for matching edge cases
ea0de32 
We don't allow url encoded hosts but do permit ports with leading 0s.
w3c/webappsec-csp#597
w3c/webappsec-csp#596

Bug: 1418009
Change-Id: Ie8ddc509b63e1aa9d35d4e2b989df63483bfca6a
annevk
annevk reviewed Apr 21, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 22, 2023
@arichiv @chromium-wpt-export-bot
[CSP] WPTs for matching edge cases
d83c7fe 
We don't allow url encoded hosts but do permit ports with leading 0s.
w3c/webappsec-csp#597
w3c/webappsec-csp#596

Bug: 1418009
Change-Id: Ie8ddc509b63e1aa9d35d4e2b989df63483bfca6a
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 24, 2023
@arichiv @chromium-wpt-export-bot
[CSP] WPTs for matching edge cases
fb9ece2 
We don't allow url encoded hosts but do permit ports with leading 0s.
w3c/webappsec-csp#597
w3c/webappsec-csp#596

Bug: 1418009
Change-Id: Ie8ddc509b63e1aa9d35d4e2b989df63483bfca6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4454670
Reviewed-by: Dustin Mitchell <djmitche@chromium.org>
Commit-Queue: Ari Chivukula <arichiv@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1134816}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 24, 2023
@arichiv @chromium-wpt-export-bot
[CSP] WPTs for matching edge cases
4f8df7a 
We don't allow url encoded hosts but do permit ports with leading 0s.
w3c/webappsec-csp#597
w3c/webappsec-csp#596

Bug: 1418009
Change-Id: Ie8ddc509b63e1aa9d35d4e2b989df63483bfca6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4454670
Reviewed-by: Dustin Mitchell <djmitche@chromium.org>
Commit-Queue: Ari Chivukula <arichiv@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1134816}
@arichiv arichiv requested a review from annevk April 27, 2023 09:15
annevk
annevk reviewed Apr 27, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@arichiv arichiv requested a review from annevk April 27, 2023 12:10
@annevk annevk mentioned this pull request Apr 27, 2023
Closed
annevk
annevk reviewed Apr 27, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@arichiv arichiv requested a review from annevk April 27, 2023 14:07
annevk
annevk reviewed Apr 27, 2023
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antosart should have a look at this as well.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
arichiv and others added 3 commits April 27, 2023 12:06
@arichiv @annevk
Update index.bs
31151c7 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@arichiv @annevk
Update index.bs
5723cb0 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@arichiv
Update index.bs
ee36eed
@arichiv arichiv requested a review from antosart April 27, 2023 16:13
antosart
antosart reviewed Apr 28, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@arichiv arichiv requested a review from antosart May 2, 2023 11:46
@annevk
Copy link
Member

annevk commented May 2, 2023

@arichiv what did you think of making it accept a URL to compare against instead?

@arichiv
Copy link
Member Author

arichiv commented May 2, 2023

@arichiv what did you think of making it accept a URL to compare against instead?

It does now, I just updated it

@antosart
Copy link
Member

antosart commented May 2, 2023

(The build error is unrelated, I am fixing it in #602)

antosart
antosart reviewed May 3, 2023
View reviewed changes
Copy link
Member

@antosart antosart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this ended up using url directly. I'll wait if @annevk has other comments, but this looks good to me.

annevk
annevk approved these changes May 3, 2023
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some tiny nits.

String formatting should maybe also consistently be "..." rather than sometimes "..."?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
arichiv and others added 2 commits May 3, 2023 04:52
@arichiv @annevk
Update index.bs
a7f8426 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@arichiv @annevk
Update index.bs
6445448 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@arichiv arichiv merged commit ea230bc into main May 3, 2023
2 checks passed
@arichiv arichiv deleted the port-matching branch May 3, 2023 08:54
github-actions bot added a commit that referenced this pull request May 3, 2023
@arichiv @github-actions
Revise port matching logic (#596)
2fbc0c2 
SHA: ea230bc
Reason: push, by arichiv

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 22, 2023
@arichiv @moz-wptsync-bot
Bug 1829211 [wpt PR 39631] - [CSP] WPTs for matching edge cases, a=te…
c72de66 
…stonly

Automatic update from web-platform-tests
[CSP] WPTs for matching edge cases

We don't allow url encoded hosts but do permit ports with leading 0s.
w3c/webappsec-csp#597
w3c/webappsec-csp#596

Bug: 1418009
Change-Id: Ie8ddc509b63e1aa9d35d4e2b989df63483bfca6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4454670
Reviewed-by: Dustin Mitchell <djmitche@chromium.org>
Commit-Queue: Ari Chivukula <arichiv@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1134816}

--

wpt-commits: 4f8df7ab83ab1562214fcca2f67bb1718cc28808
wpt-pr: 39631
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this pull request May 23, 2023
@arichiv @moz-wptsync-bot
Bug 1829211 [wpt PR 39631] - [CSP] WPTs for matching edge cases, a=te…
dcc1a03 
…stonly

Automatic update from web-platform-tests
[CSP] WPTs for matching edge cases

We don't allow url encoded hosts but do permit ports with leading 0s.
w3c/webappsec-csp#597
w3c/webappsec-csp#596

Bug: 1418009
Change-Id: Ie8ddc509b63e1aa9d35d4e2b989df63483bfca6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4454670
Reviewed-by: Dustin Mitchell <djmitche@chromium.org>
Commit-Queue: Ari Chivukula <arichiv@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1134816}

--

wpt-commits: 4f8df7ab83ab1562214fcca2f67bb1718cc28808
wpt-pr: 39631
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antosart antosart antosart left review comments

@annevk annevk annevk approved these changes

Assignees

@arichiv arichiv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

port-part matches issues
3 participants
@arichiv @annevk @antosart