-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
javascript: support custom header name for source maps #4630
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.
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
javascript: support custom header name for source maps #4630
Conversation
Security concerns found
Generated by 🚫 danger |
94f465d to
61b3b28
Compare
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.
These patterns are effectively arbitrary and likely over conservative, but should be sufficient for anyone using this and prevents someone putting in complete garbage.
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.
Basically you can basic auth this now. Might make sense to mention.
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.
Should there really be a length limit here? JWT tokens can be very long
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.
There is some limit. How long are you thinking? The value can't be say, 1MB long. :) I picked 40 as an arbitrary value since our auto generated value is a uuid which is 32 characters.
Just wondering what case you'd want to shove a JWT here.
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.
Hmm, I guess if the goal is with basic auth, it'll probably need to be longer than 40 chars, and also accept a wider set of characters since it's base64 encoded.
Lemme revisit this and get a better pattern here. Thanks for bringing it to my attention. :)
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.
Alright, I updated the regular expression here to actually accommodate basic auth and allow base64 characters and spaces and a length of 255. Unclear if we'd need more than that, but this seems reasonable now.
61b3b28 to
b7ddfe8
Compare
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.
Basically you can basic auth this now. Might make sense to mention.
|
@mattrobenolt this is looking good! #4500 will also need fixing otherwise it's too dangerous to use basic auth |
|
@graingert I agree, and I'll get #4500 addressed. In the meantime, gonna get this merged. :) |
b7ddfe8 to
5e5765c
Compare
This is being carried from GH-4017.