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: Fix Referrer getter for malformed data + test #210

Closed
wants to merge 1 commit into from

Conversation

jakubboucek
Copy link
Contributor

@jakubboucek jakubboucek commented Feb 17, 2022

  • bug fix
  • BC break? no

What problem is PR solving

Method \Nette\Http\Request::getReferer() throws LogicException when is called HTTP Request with malformed Referer header.

curl -v -H 'Referer://///' https://example.com/
$httpRequest->getReferer();
// throws InvalidArgumentException: Malformed or unsupported URI '/////'.

It's wrong, because:

  • Exception breaks Application stability according to 🔴 User's input,
  • Exception is Logic type, but origin of error is only invalid user's input – developer here can't to prevent throw exception,
  • Exception is throws too late, when developer call getter – until then it is Request object at incostistence,
  • Developer does not except exception here (but be aware to just add @throws, it doesn't enough),
  • Because Referer is nullable and because most usage of Referer value is logging of processed request, I suppose exception throwing is non-sense here.

Normal use of Referer value is fo logging:

$logger->logEvent($result, $id, (string)$request->url, $request->method, (string)$request->referer);

We can't to wrap whole row to try/catch, because it cause to skip log event. Developer must to separate getter call:

try {
    $referer = (string)$request->referer;
}
catch(Nette\InvalidArgumentException $e) {
    $referer = null;
}

$logger->logEvent($result, $id, (string)$request->url, $request->method, $referer);

What PR suggest

PR is suggesting to transmute Exception to Notice, return null and never throw Exception.

Developer always can to read raw header by access through $request->getHeader('Referer');

Security

Potentially this bug can be abused to intentionally bypassing log of user's activity, because in most cases is Referer value used for auditing done operations.

@dg
Copy link
Member

dg commented Feb 17, 2022

I want to remove this method completely because it makes no sense. Who uses the Referer these days?

@jakubboucek
Copy link
Contributor Author

jakubboucek commented Feb 17, 2022

True true, @deprecated annotation looks great solution. But still this fix is important to prevent intentional break app by input data.

@dg
Copy link
Member

dg commented Feb 17, 2022

Sure, there is a getHeader('Referer') method. But no extra method is needed for this header.

@jakubboucek jakubboucek force-pushed the fix-request-referer branch from af123a4 to b182d4e Compare March 2, 2022 03:52
@dg
Copy link
Member

dg commented Apr 2, 2022

Even duckduckgo, that is privacy-oriented, sets the referer:

The referer doesn't send the web, your browser does. But a lot of extensions and proxies block it nowadays. That's why the Origin header was created as a successor.

dg added a commit that referenced this pull request Apr 2, 2022
@dg
Copy link
Member

dg commented Apr 2, 2022

Okay, let me rephrase that. The Referer header is now an old-fashioned unreliable thing, which therefore doesn't deserve to have its own method in the fairly simple Http\Request interface. And if the current method is not well usable, I can in good conscience give it away and let the users solve it how they want in userland.

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.

2 participants