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.

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

Request: Move Basic Auth credential from Url to Request due to prevent leak it #211

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

jakubboucek
Copy link
Contributor

@jakubboucek jakubboucek commented Mar 1, 2022

Currently is too risky use Url object from from Http\Request created from global, because it can simply cause to leak user's credentials:

echo $httpRequest->getUrl();

// https://example.com@username:password/index.php

The Url object already provide method –>withoutUserInfo() to remove user credential, but when programer forgot to use that, the credentials can leaks.

This PR ir removing user's credentials from Url object and move it to Http\Request directly.

It's based on czech discussion at Nette Forum: https://forum.nette.org/cs/33127-http-basic-login-http-url-nette-3-security-issue

I know, it's too naive PR, here is missing any way to notify developer the Url->getUser() no more get user from current request. This PR sent as initiation of change insecurity.

@dg dg merged commit e99694a into nette:master Mar 1, 2022
dg pushed a commit that referenced this pull request Mar 1, 2022
@jakubboucek jakubboucek deleted the basic-auth-to-request branch March 2, 2022 03:49
dg pushed a commit that referenced this pull request Apr 12, 2022
dg pushed a commit that referenced this pull request Sep 6, 2022
dg pushed a commit that referenced this pull request Oct 13, 2022
dg pushed a commit that referenced this pull request Oct 13, 2022
dg pushed a commit that referenced this pull request Oct 13, 2022
dg pushed a commit that referenced this pull request Nov 24, 2022
dg pushed a commit that referenced this pull request Nov 24, 2022
dg pushed a commit that referenced this pull request Nov 24, 2022
dg pushed a commit that referenced this pull request Nov 24, 2022
dg pushed a commit that referenced this pull request Nov 25, 2022
dg pushed a commit that referenced this pull request Nov 25, 2022
dg pushed a commit that referenced this pull request Nov 27, 2022
dg pushed a commit that referenced this pull request Nov 27, 2022
dg pushed a commit that referenced this pull request Nov 27, 2022
dg pushed a commit that referenced this pull request Nov 27, 2022
@carloscz
Copy link

carloscz commented Dec 7, 2022

this should be released as new major to comply with semver :/

@dg
Copy link
Member

dg commented Dec 27, 2022

@carloscz There were security reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants