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

nameless cookie causing hapi fail parsing #2957

Closed
davidhouweling opened this issue Nov 25, 2015 · 16 comments
Closed

nameless cookie causing hapi fail parsing #2957

davidhouweling opened this issue Nov 25, 2015 · 16 comments
Assignees
Milestone

Comments

@davidhouweling
Copy link

@davidhouweling davidhouweling commented Nov 25, 2015

I know that cookies should always have a name, but i stumbled across an issue where something injected a nameless cookie into my code (probably a third party library). But as a result, it causes the request.state to equal null.

To test this, host a site using hapi, in Firefox console...

document.cookie='=1'

Then reload the page (i also set my hapi server.route's config.state.failAction to 'log'), and you get the following in the console/terminal

Internal request error { header: '1', errors: null }

Needs to be a way to tell hapi to drop any invalid cookies like these.

I'm presently using hapi version 9.0.3, so i understand it is a little out of date.

Let me know if this has been fixed in a later version. If so i will look to upgrading.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 25, 2015

Did you try configuring strict mode to false?

@davidhouweling
Copy link
Author

@davidhouweling davidhouweling commented Nov 25, 2015

@hueniverse I don't see how I could do this with a nameless cookie though?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 26, 2015

Set the connection state default to strictHeader: false

@davidhouweling
Copy link
Author

@davidhouweling davidhouweling commented Nov 26, 2015

Unfortunately no luck. This is a stripped down version of my setup.

const server = new Hapi.Server({
  connections: {
    state: {
      strictHeader: false
    }
  }
});
server.on('request-internal', (request, event, tags) => {
  if (tags.error && tags.state) {
    console.error('Internal request error', event.data);
    server.methods.log.error(event.data);
  }
});

and inside server.register

  server.route({
    method  : 'GET',
    path    : '/',
    handler : (request, response) => {
      console.log(request.state); // this will result in null
    },
    config: {
      state: {
        failAction: 'log'
      }
    }
  });

Still yields in the same result.

Internal request error { header: '1', errors: null }
null

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 26, 2015

Yeah...turns out even the non strict mode requires a key=value pair. I am not going to change the code to handle this case. You should not allow third-party code to crap all over your cookies. If you can't help it, I suggest you do some light regex parsing of the cookie header in an onRequest and remove the offending value from request.headers.cookie and then let the parse take over from there.

@hueniverse hueniverse closed this Nov 26, 2015
@hueniverse hueniverse self-assigned this Nov 26, 2015
@davidhouweling
Copy link
Author

@davidhouweling davidhouweling commented Nov 26, 2015

okay thanks for your feedback, will put something together for that. And yes, I do agree with you that it is quite dodgy. But it does also open up hapi to being broken by malicious browser extensions.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 26, 2015

Browser extensions add cookies to random namespaces they have nothing to do with?

@codedemonuk
Copy link

@codedemonuk codedemonuk commented Nov 27, 2015

It would make Hapi far more robust is you would be more liberal in supporting cookies, parsing the ones that can be parsed and ignoring the ones that are malformed.

There could be other code from other applications running on the same domain that set an invalid cookie which then gets passed down to Hapi because it is powering another application on the same domain.

@fabiosantoscode
Copy link

@fabiosantoscode fabiosantoscode commented Nov 27, 2015

In general I agree that being liberal in what you accept and being strict in what you send is a good thing to try to achieve.

You don't even need a malicious browser extension, it appears that if any crappy third-party advertisement script evaluates document.cookie = '=lel' it will stop the user from visiting the website again until they clear their cookies.

Or you yourself might accidentally create a nameless cookie on the client, push it to production without realizing, and stop your users from visiting the website again until they clear their cookies.

Sorry for butting in, just thought I'd put this out there.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 27, 2015

Where do you draw a line? missing name is ok? missing = is ok? Do you just hope for the right outcome when you parse bad strings? fail the entire cookie? this is not about being liberal in what you accept, it is about being unable to properly parse a broken string. I am open to making adjustments but is this the right line to draw?

@fabiosantoscode
Copy link

@fabiosantoscode fabiosantoscode commented Nov 27, 2015

This is just my opinion, and I can also see excellent arguments made against it as well.

Where to draw the line is a very philosophical and ultimately very personal question, and I wouldn't be as presumptuous as to tell you whether to be strict or loose.

It is, after all, not my project.

I just thought I would share my point of view. In general I am pretty scared and aware of the messy JavaScript on the wild wild web, between browser hacks, third party code or just plain legacy, and what I really think is that if a website can be made to stop working for a certain client with only pure sandboxed JavaScript then it is certainly something that deserves a fix. Either it be for trolling, taking down a website for an agenda or just a plain old mistake, this can happen.

In short, if I were you I would push the line a bit towards a liberal interface so as to provide more robustness, but do not wish to tell you what to do.

All the best :)

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 27, 2015

I asked some specific questions, I wasn't trying to make a point. If you can offer a specific change, I am open to it.

@fabiosantoscode
Copy link

@fabiosantoscode fabiosantoscode commented Nov 27, 2015

Personally, I would add to the object the empty string as the key as well.

Not to fully support it, but to document what happened when someone comes to console.log the entire cookie hash to see why their cookie is not there.

@hueniverse hueniverse reopened this Nov 28, 2015
@codedemonuk
Copy link

@codedemonuk codedemonuk commented Nov 28, 2015

I would support Fabios suggestion for how it could be handled.

@davidhouweling
Copy link
Author

@davidhouweling davidhouweling commented Nov 30, 2015

I also would support @fabiosantoscode suggestion.

In regards to my previous example, I think the best example we can give which is quite real world is the presence of ads on our website. All we are told is to drop the ad framework in, and have a placeholder for where the ad goes. The ad framework runs its own javascript on our page, the ads themselves run their own javascript (or better yet, flash, though that is going out of fashion). Any of these dependencies could do something stupid like set a nameless cookie. They may have tested it on their end and not seen an effect (because all they care about is the ad working, and they don't have a server side to care about), but as a result it could easily bring down any site with hapi on the server side.

@hueniverse I understand you don't want to drift away from the standard and it is quite difficult to draw a line, but they should be assessed on a case by case basis.

@hueniverse hueniverse added this to the 12.0.0 milestone Jan 2, 2016
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 2, 2016

Implemented in hapijs/statehood#15

@hueniverse hueniverse closed this Jan 2, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants