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

Add session hijacking mitigation configuration #564

Merged
merged 15 commits into from
Jul 25, 2023

Conversation

Slamdunk
Copy link
Member

No description provided.

@Slamdunk
Copy link
Member Author

Cmon @Ocramius
image

@Slamdunk Slamdunk requested a review from Ocramius April 4, 2023 06:21
@Slamdunk Slamdunk changed the title Add session hijacking mitigation middleware Add session hijacking mitigation configuration Apr 5, 2023
@Slamdunk
Copy link
Member Author

Slamdunk commented May 2, 2023

Ping @Ocramius

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Overall, patch direction improved massively.

My feedback is mostly highlighting problems without proposing real alternatives at the moment, since I mostly focused on providing some feedback at first.

I believe we can discuss this on a call again, but the fact that most validation is now handled inside lcobuccci/jwt is really good.

Things I don't like:

  • current ergonomics and docs (mostly, because we're uncovering issues due to lack of a config builder)
  • introduction of sodium_ stuff that we'll have to maintain, where a simple sha1 is already effective
  • reliance on $_SERVER where reading a header would be preferable
  • reliance on REMOTE_ADDR where unreliable

src/Storageless/Http/ClientFingerprint/Configuration.php Outdated Show resolved Hide resolved
Comment on lines 9 to 12
interface Source
{
/** @return non-empty-string */
public function extractFrom(ServerRequestInterface $request): string;
Copy link
Member

Choose a reason for hiding this comment

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

This interface requires some documentation: taken as-is, it is not very clear what it does.

Also, could it be that it cannot extract information from $request?

README.md Show resolved Hide resolved
src/Storageless/Http/ClientFingerprint/UserAgent.php Outdated Show resolved Hide resolved
/** @immutable */
final class RemoteAddr implements Source
{
public const REQUEST_ATTRIBUTE_NAME = 'REMOTE_ADDR';
Copy link
Member

Choose a reason for hiding this comment

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

There is a problem with relying on REMOTE_ADDR: it does not consider any proxies. If we assume that there's an upfront middleware that clears all this information, that's fine, but this Source is unreliable when the middleware is deployed independently, and put behind a reverse proxy.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing this library can do about that is documentation:
https://github.com/psr7-sessions/storageless/tree/9.0.x#session-hijacking-mitigation

@Slamdunk
Copy link
Member Author

@Ocramius sorry to bother you: we really need this feature. I've opened #570 to address many of the review comments you've given here, but I didn't see any reply here nor there.

If you don't have time anymore, I propose myself as a Collaborator for this package to evolve and maintain it.

@Ocramius
Copy link
Member

Done :-)

@Slamdunk Slamdunk changed the base branch from 8.17.x to 9.0.x July 24, 2023 12:23
@Slamdunk Slamdunk self-assigned this Jul 25, 2023
@Slamdunk Slamdunk added this to the 9.0.0 milestone Jul 25, 2023
@Slamdunk Slamdunk merged commit ebbafea into psr7-sessions:9.0.x Jul 25, 2023
10 checks passed
@Slamdunk Slamdunk deleted the hijacking_mitigation branch July 25, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants