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

http: handle bad url-encoded Cookie session token #1269

Closed
wants to merge 7 commits into from
Closed

http: handle bad url-encoded Cookie session token #1269

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
62eba7f
http: handle bad url-encoded Cookie session token
Nov 4, 2024
dd0dd40
Merge branch 'master' into http-preprocessor-url-decode
Nov 9, 2024
a32c646
ramda
Nov 9, 2024
9663c3c
lint
Nov 9, 2024
97d5d94
update test name
Nov 9, 2024
419fd64
Merge branch 'master' into http-preprocessor-url-decode
Nov 10, 2024
2fa94cb
move comments
Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/http/preprocessors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,10 @@ const authHandler = ({ Sessions, Users, Auth }, context) => {

// actually try to authenticate with it. no Problem on failure. short circuit
// out if we have a GET or HEAD request.
alxndrsn marked this conversation as resolved.
Show resolved Hide resolved
const maybeSession = authBySessionToken(decodeURIComponent(token));
const decodedToken = urlDecode(token);
alxndrsn marked this conversation as resolved.
Show resolved Hide resolved
if (decodedToken.isEmpty()) return reject(Problem.user.authenticationFailed());

const maybeSession = authBySessionToken(decodedToken.get());
if ((context.method === 'GET') || (context.method === 'HEAD')) return maybeSession;

// if non-GET run authentication as usual but we'll have to check CSRF afterwards.
Expand Down
24 changes: 24 additions & 0 deletions test/unit/http/preprocessors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,30 @@ describe('preprocessors', () => {
context.auth.session.should.eql(Option.of(new Session({ test: 'session' })));
}));

it('should reject cookie which cannot be url-decoded', () => {
let caught = false;
Promise.resolve(authHandler(
Copy link
Member

Choose a reason for hiding this comment

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

I think this promise needs to be returned in order for Mocha to wait on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very possible. Do tests on line 188 and 208 need the same change?

Copy link
Member

Choose a reason for hiding this comment

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

Ah good catch, that sounds right to me. If there's an assertion in a promise chain, that assertion will only affect the success/failure of the test if the promise is returned to Mocha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing tests fixed at #1286

{ Auth, Sessions: mockSessions('alohomora') },
new Context(
createRequest({
method: 'GET',
headers: {
'X-Forwarded-Proto': 'https',
Cookie: 'session=aloho%25eamora'
},
cookies: { session: 'aloho%eamora' },
url: ''
}),
{ auth: { isAuthenticated() { return false; } }, fieldKey: Option.none() }
)
)).catch((err) => {
err.problemCode.should.equal(401.2);
caught = true;
}).then(() => {
caught.should.equal(true);
});
});

describe('CSRF protection', () => {
const mockSessionsWithCsrf = (expectedToken, csrf) => ({
getByBearerToken: (token) => Promise.resolve((token === expectedToken)
Expand Down