Cookie-prefix should fix phabricator instances where x.com and x.y.com have conflicting cookie names
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- Restricted Diffusion Commit
rPe6a6c265b04c: Aprhont - Adding cookie-prefix, as config option, and into cookie methods
Pushed branch to dev.phab.example.com, logged into phab.example.com and into dev.phab.example.com.
Diff Detail
Diff Detail
- Branch
- cookieprefix
- Lint
Lint Passed - Unit
Tests Passed
Event Timeline
Comment Actions
Biggest concerns:
After I diff'ed this I realized there's probably tests somewhere that I need to add
Wasn't sure how to properly minimize the number of getEnvConfigs; originally was hoping to not have to edit getCookie / clearCookie but that didn't work out as nicely as I hoped.
Lastly, wondering if null is a solid default config. It seems to best fit a least amount of changes paradigm, but it might be better off using a random string? Or base-uri? This would prevent cookie naming collisions on phab.example.com / dev.phab.example.com, and in the case of load balanced instances where multiple nodes are hosting phab.example.com local config (normally) would be shared, and so would db. Thus would still not require config of this option.
Comment Actions
Minor things:
- If the prefix has strlen(), let's add a separator character (like _) so that it's not possible for cookie bc from namespace a to collide with cookie abc from no namespace..
- By convention, prefer $a.$b over $a . $b (no space around . operator). I thought we had lint for this already, but maybe not.
- It would be slightly cleaner to add a setCookiePrefix() method and call it from AphrontDefaultApplicationConfiguration->buildRequest().
- I don't think you really need to add tests for this as long as you tested locally.
- We should not namespace by default. If nothing else, this will break every session on every install everywhere, and doesn't seem like it provides much benefit. I think this use case is just barely reasonable as-is, and putting a multitude of installs behind some weird LB thing should definitely be done in a more thoughtful manner (we have substantial planning around our own intents to do this).
| src/applications/config/option/PhabricatorCoreConfigOptions.php | ||
|---|---|---|
| 81–82 | For consistency, add a period to make this a full sentence. | |
Comment Actions
- Style changes regarding concatenation, typos
- Added helper methods in hopes of being a bit cleaner
Comment Actions
A couple more inlines, this is 90% of the way there. D7988 should fix the lint issue, which was a bug in arc.
| src/aphront/AphrontRequest.php | ||
|---|---|---|
| 277–279 | With the above implementation, this can then be simplified to: $name = $this->getPrefixedCookieName($name); If there's no prefix, it will just return $name unmodified. If there is a prefix, it will apply the prefix correctly. | |
| 282 | Implement this as: if (strlen($this->cookiePrefix)) {
return $this->cookiePrefix.'_'.$name;
} else {
return $name;
}This method should also be private. | |
| 283–285 | As above. | |
| 349–351 | As above. | |
| src/aphront/configuration/AphrontDefaultApplicationConfiguration.php | ||
| 110 ↗ | (On Diff #18069) | This is the syntax error: missing semicolon at the end of this line. |
| src/applications/config/option/PhabricatorCoreConfigOptions.php | ||
| 82 | For consistency, add a period at the end of the sentence: ...cookie names.
^Other summary text uses this style. | |