This repository has been archived by the owner on Nov 6, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Detect static securecookies to trivialize more rules #16029
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
77f08cf
Detect static securecookies to trivialize more rules
ca2844f
Add comments on static cookie terminology
4bbd731
Remove securecookie tags if they are not effective
ca52ba2
Add comments on the safety removing dangling securecookies
5c149aa
Fix a bug for handling the dangling securecookie rules
a37df21
semistandard --fix explode-regexp.js trivialize-rules.js
db2b8b6
Revert "semistandard --fix explode-regexp.js trivialize-rules.js"
e8f27ee
Remove unused variables and fix syntax manually
6e82204
Update comments and remove unnecessary checks
e5ad3c5
Update comments on partial dangling securecookie
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sufficient munging to ensure it matches the target domains?
If not, at worst it will not match and result in a
false
result, which isn't terrible but not desirable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hainish this should be good enough. As mentioned in #16029 (comment), securecookie.host only match cookie.domain if there is a rule covers cookie.domain. Given the
target
are trivial, cookie.domain cannot be anything other thandomain.example.com
and.domain.example.com
for a securecookie rule to works. With condition (1) effective, securecookie.host should explode to either one of them. Otherwise, the securecookie will never be applied (dangling rules?) under the current implementation.With the above logic in mind, I have also add 4bbd731 to remove some of these securecookie rules that will never be effective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it would be great to have #15998 and #16000 merged before running this against the master.