Don't store request specific data into the Layer object

[Congelli501]

Issue

Currently, the path & params of the latest request is stored into the last matched layer. The "Layer" object should be immutable: a request should not modify its attributes.

This means that parameters are kept into memory after the end a request.

Fix

This PR simply removes the path & params parameters of the Layer object, and return them as the result of the match function.

Test case

A test case is included, to check that the params attribute is not available anymore in the Layer.

History

An initial PR was made on the express project: https://github.com/expressjs/express/pull/5426

Example

For example, this code will show the latest value of the parameter, as the value is stored until a new request is made:

const express = require('./index.js');
const app = express();
const port = 3000;

app.get('/secret/:token', (req, res) => {
	res.send('Hello World!');
});

app.listen(port, () => {
	console.log(`Example app listening on port ${port}`);
});

setInterval(() => {
	console.log(app._router.stack.at(2).params);
}, 1000);

curl localhost:3000/secret/super_secret

Result:

Example app listening on port 3000
undefined
undefined
undefined
{ token: 'super_secret' }
{ token: 'super_secret' }

[wesleytodd]

I just want clarify one thing: putting a secret in the URL is never acceptable. Every proxy layer, access log system, and tracing implementation will store those in plain text and often ship them off the instance to log aggregation systems. I am not opposed to the general idea of not keeping around information about completed requests, but if the purpose of this request is some "security" thing like it was tagged with when originally opened then I think we need to be clear that nothing in the URL is considered secure or secret.

[Congelli501]

I just want clarify one thing: putting a secret in the URL is never acceptable. Every proxy layer, access log system, and tracing implementation will store those in plain text and often ship them off the instance to log aggregation systems. I am not opposed to the general idea of not keeping around information about completed requests, but if the purpose of this request is some "security" thing like it was tagged with when originally opened then I think we need to be clear that nothing in the URL is considered secure or secret.

Putting a secret in a URL should be avoided when possible, but there are a lot of use cases where you don't have a choice. Any reset password or magic connection link sent by email for example will have a token in the URL, and there is no other way to do it.

Even if there was no legitimate use cases for putting secrets in an URL, mitigating user mistakes (developers are a user of the lib) is always a good thing. Take this far fetched example: It would be like saying that storing hashed password is not important because users should use a password manager and a different password for each service.

I agree that this is a very low security concern, anyone having the capability to read a token from the express service would obviously have others means to do so at disposal.

FYI, I was looking for memory leak in an application when I found this behaviour in express, as some new string / objects are stored on each request (replacing the previous one, so this is not a memory leak, it was just flagged by the memory profiler).

[wesleytodd]

Any reset password or magic connection link sent by email for example will have a token in the URL

This is an application design choice, and if you are going to make that choice then you need to be aware of the potential consequences. In this case, the consequences go far beyond Express keeping a reference around in memory. For the sake of this PR any security discussion points are straw man arguments. If an attacker has an RCE or access to the process memory then they have more than your individual users password reset link as well. This is just not at all a security concern for the express project even in a "protect users from themselves" sort of way.

I am not saying we should not clean this up when the request is finished. Sorry for being so pedantic, we just don't want someone who doesn't know what is going on to see this and be concerned for no reason (or worse, file a CVE we need to fight).

[wesleytodd]

I should add, all of the above comments still need to be addressed to consider moving this forward. Please clean up all of the extra changes and slim this down to just the change to clean up the state.

[Congelli501]

@wesleytodd, do you mean this comment?

We can land this in 2.0 since folks reach into these layer parameters currently.

I don't really see a way to clean the sate without breaking these use cases.

An obvious way would be to just reset the state (this.params & this.path on Layer) back to empty values at some point, but I don't see where that could be added: these attributes are currently kept with the last request value as is as long as non async code is run (as they can be overwritten by another request). This means that, even if we add a cleanup code after this line (https://github.com/pillarjs/router/blob/master/index.js#L350), it can still break existing usage of this.params & this.path.

I'm not familiar with the express codebase so there might be something I'm missing here.

[wesleytodd]

EDIT: I didn't notice you had pushed some of the things which were requested to be changed. With this slimmed down part I see maybe those swaps were because you set match to false. Let me re-visit the code now that you did take a cleanup pass, sorry I didn't notice that at first since there were other conversation threads I was focused on.

do you mean this comment?

No I meant all the unrelated stuff like swapping !== checks to === checks and stuff that doesn't have anything to do with the change you are proposing.

An obvious way would be to just reset the state (this.params & this.path on Layer) back to empty values at some point, but I don't see where that could be added

Yeah, I agree we might not be able to find a place to clear those out that is not a breaking change. Either way, I and other contributors are not going to spend a bunch of time helping find that while the PR could never be merged because of all the unrelated changes. Once this is slimmed down to just the changes you need to achieve the goal we can discuss options for landing it.

[Congelli501]

No I meant all the unrelated stuff like swapping !== checks to === checks and stuff that doesn't have anything to do with the change you are proposing.

I re-checked the PR, and all the changes I made are only related to the removal of the state. The match function now returns either an object on success or false on failure (no match), so I need to check for === false instrad of !== true.

[wesleytodd]

@Congelli501 yeah sorry I had not fully re-reviewed the code before making that comment. I edited as soon as I noticed that. Sorry if that was confusing.