Reduce discoverability of session cookie name and add RFC 6265 support

[summercms]

While researching a different issue, I came across this post: laravel/laravel#4305

Laravel link: https://github.com/laravel/laravel/blob/5.8/config/session.php#L127-L130

I leave this open for the admins to discuss and look deeper into this.

[LukeTowers]

@ayumi-cloud while I appreciate the thought, we're not going to make this change, we don't include env() calls in our default config so that the project is more approachable to developers not familiar with Laravel (we do support them, we just don't include it by default, we have the october:env command to switch out config files to use env() calls).

I would say that this is more of a neat practice that individual projects can decide whether or not they want to enable it, so if you would like to submit this to https://octobertricks.com/ then I would recommend doing that instead.

[Samuell1]

I think this should done when ocms will be updated to laravel 6.0 version. (I done some changes already in my fork: Samuell1@7598cfa )

@LukeTowers I think maybe this can be exception, because env values have callback to other option if they are not defined.

[LukeTowers]

@Samuell1 I don't see the point. The only reason to make this change is one of security through obscurity by trying to prevent people from detecting what platform is running by looking at the cookies generated. There's so many other ways to identify that October is in use that it seems pointless to me.

[Samuell1]

@LukeTowers Thats true, but i think configs that are inherited from laravel should correspodent with laravel configs to make it simplier.

[LukeTowers]

@Samuell1 simpler for who exactly? That's a considerably more complex value than the current one that can be understand easily enough by us having this discussion but isn't immediately obvious as to what the purpose of it is to anyone else that's not familiar with why anyone would want to have that value. For that reason, the reason that it doesn't really add any value out of the box for most users, and the fact that individual projects using October can always choose to put whatever they want there themselves I don't think we'll reconsider adding it.

[summercms]

@LukeTowers @Samuell1 I wanted to ask, instead of making the config messy, could this RFC 6265 support just be included into the parser side?

[LukeTowers]

@ayumi-cloud I have no idea what you're talking about

[summercms]

@LukeTowers

In RFC 6265, the cookie name is specified as an RFC 2616 token, which means you can pick from the alphanums plus: !#$%&'*+-.^_`|~

So what I was asking is, does October escape the cookie name in the parser, according to the RFC 6265 protocol?

Basically, add the RFC 6265 whitelist to October's parser for escaping the cookie name.

[LukeTowers]

@ayumi-cloud October does whatever Laravel does, so if you want to check that, check Laravel's code.

[summercms]

@LukeTowers laravel doesn't do it at the parser level, they do it direct in the config file, which makes no sense to me. It would make more sense to do all the escaping at the same level. As October doesn't use the .env in the configs to make it easier for newbie's. October could create a more cleaner approach by moving it into the parser level.

p.s. I'm talking about the code line here: https://github.com/laravel/laravel/blob/c4a7a7bc9cdccd00d6fdda8846d1660a0608d7ef/config/session.php#L129

I don't understand why Laravel puts a Str:: in the config when it can be cleaner to do the same thing in the parser instead.

Of course totally up to you.

(Hope you get what I mean)

---

[edit]

It looks like the cookie name is just going straight in here: https://github.com/illuminate/session/blob/a95be1a3bf2cc26256b094c637813331dae56cd4/Middleware/StartSession.php#L155

and

https://github.com/illuminate/session/blob/a95be1a3bf2cc26256b094c637813331dae56cd4/Middleware/StartSession.php#L94

Looks like nothing in October could stop me adding funny characters into the cookie name as nothing is being escaped.

The point of the RFC 6265 is to escape the cookie name.

[summercms]

@LukeTowers

I'm saying this:

Laravel does this

October does this

I'm asking should it be like this

[Samuell1]

@ayumi-cloud Its just config value it doesnt send your cookie name there. it only uses october_session cookie name.

[summercms]

@Samuell1 my point is that the cookie name spec allows certain characters, right now October is not filtering them at all.

See here what I mean: https://stackoverflow.com/questions/1969232/allowed-characters-in-cookies

[LukeTowers]

@ayumi-cloud I really don't think it's a concern TBH, if a developer decides to be goofy and change the session name to something with disallowed characters then it's on them to identify and fix that since there is no reason for them to do so.