-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
http: decode username and password before encoding #31450
Changes from all commits
17ef5bf
76ca736
8ccf69f
5ebbb79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1265,6 +1265,10 @@ function domainToUnicode(domain) { | |||||
return _domainToUnicode(`${domain}`); | ||||||
} | ||||||
|
||||||
function decodeAuth(str) { | ||||||
return decodeURIComponent(str).replace(':', '%3A').replace('/', '%2F'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think you need to use global replace at least ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax This is still unaddressed (and, apparently, untested.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be unexpected outcome with undefined There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const forwardSlashRegEx = /\//g;
+const colonRegEx = /:/g;
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only thing |
||||||
} | ||||||
|
||||||
// Utility function that converts a URL object into an ordinary | ||||||
// options object as expected by the http.request and https.request | ||||||
// APIs. | ||||||
|
@@ -1284,7 +1288,7 @@ function urlToOptions(url) { | |||||
options.port = Number(url.port); | ||||||
} | ||||||
if (url.username || url.password) { | ||||||
options.auth = `${url.username}:${url.password}`; | ||||||
options.auth = `${decodeAuth(url.username)}:${decodeAuth(url.password)}`; | ||||||
} | ||||||
return options; | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const http = require('http'); | ||
const MakeDuplexPair = require('../common/duplexpair'); | ||
|
||
// Test that usernames from URLs are URL-decoded, as they should be. | ||
|
||
{ | ||
const url = new URL('http://localhost'); | ||
url.username = 'test@test"'; | ||
url.password = '123456^'; | ||
|
||
const server = http.createServer( | ||
common.mustCall((req, res) => { | ||
assert.strictEqual( | ||
req.headers.authorization, | ||
'Basic ' + Buffer.from('test@test":123456^').toString('base64')); | ||
res.statusCode = 200; | ||
res.end(); | ||
})); | ||
|
||
const { clientSide, serverSide } = MakeDuplexPair(); | ||
server.emit('connection', serverSide); | ||
|
||
const req = http.request(url, { | ||
createConnection: common.mustCall(() => clientSide) | ||
}, common.mustCall((res) => { | ||
res.resume(); // We don’t actually care about contents. | ||
res.on('end', common.mustCall()); | ||
})); | ||
req.end(); | ||
} |
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.
Missing closing
-->
.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.
The section on
http.request()
in http.md seems like a more logical place to me for this note.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.