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.

Sign up for GitHub

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

Jump to bottom

Allow uppercase letters in PHP sessionid format #336

Merged
merged 1 commit into from
Sep 2, 2015
Merged

Allow uppercase letters in PHP sessionid format #336

merged 1 commit into from
Sep 2, 2015

Conversation

ArthurHoaro
Copy link
Member

Fixes #335 - Wrong login/password since v0.5.2

Regression introduced in 06b6660

@virtualtam @nodiscc This is a critical bug. Should we recreate v0.5.2 with this commit included?

@ArthurHoaro
Allow uppercase letters in PHP sessionid format
4d30975 
Fixes shaarli#335 - Wrong login/password since v0.5.2

Regression introduced in 06b6660
@ArthurHoaro ArthurHoaro added the bug it's broken! label Sep 2, 2015
@virtualtam
Copy link
Member

Should we recreate v0.5.2

Nope. Different code versions must have different tags.

A rewritten commit thou shalt never push
-- 5th Git commandment

http://geek-and-poke.com/geekandpoke/2014/3/3/end-of-working-day

Instead, we can:

  • release Shaarli v0.5.3 with only this fix,
  • rebase the stable branch
  • move pending issues to a newly created 0.5.4 milestone ;-)

@virtualtam virtualtam added this to the 0.5.3 milestone Sep 2, 2015
@ArthurHoaro
Copy link
Member Author

Yep, let's do that.

ArthurHoaro added a commit that referenced this pull request Sep 2, 2015
@ArthurHoaro
Merge pull request #336 from ArthurHoaro/login-hotfix
67ee143 
Allow uppercase letters in PHP sessionid format
@ArthurHoaro ArthurHoaro merged commit 67ee143 into shaarli:master Sep 2, 2015
@ArthurHoaro ArthurHoaro deleted the login-hotfix branch September 2, 2015 15:55
@nodiscc
Copy link
Member

nodiscc commented Sep 2, 2015

rebase the stable branch

Does that mean a git push --force on stable? Shouldn't we just cherry-pick this commit?

@virtualtam
Copy link
Member

Cherry-picking a commit from a branch to another creates a new commit (read: with a different SHA-1 ID), which is not what we want here.

The proper way is to rebase the stable branch on the latest tagged revision, i.e. the stable branch and the latest tag point to the same Git revision.

@nodiscc @ArthurHoaro I'm still at work but available on Gitter if you need a hand to release a new Shaarli version ;-)

@nodiscc
Copy link
Member

nodiscc commented Sep 2, 2015

Related stackoverflow question

If this means force pushing to stable (as I understand it does) I am against it. Users who update through git pull and are on the latest stable will get a conflict on the next git pull (rewritten history on the remote). Just cherry pick -x. Otherwise I just don't understood well, please go ahead.

@virtualtam
Copy link
Member

The current usage of the stable branch is as a pointer to the latest released (and deemed stable) tag.

In consequence, it should simply be rebased on the latest tag when it is released (its history is kept the same as the master branch).

Not to be confused with release branches, where critical bugfixes get backported to all maintained (long-life) revisions; in this case, the commit would be cherry-picked from the mainline (master) to each branch, and most likely be amended for integration.

This branching model would not apply well to Shaarli (for now), as we don't provide release branches for long-term support (low relevance for alpha/beta releases).

virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 3, 2015
@virtualtam
Session: SHA1-hash the session ID to ensure its format
0c7481b 
Improves shaarli#306
Relates to shaarli#335 & shaarli#336

Issue:
 - the format of the value returned by `uniqid()` depends on PHP settings
 - the regex checking the session ID does not cover all cases

Fix:
 - apply a hash function to the session ID (SHA1)

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
@virtualtam virtualtam mentioned this pull request Sep 3, 2015
Merged
@Cy4n1d3 Cy4n1d3 mentioned this pull request Sep 4, 2015
Closed
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 4, 2015
@virtualtam
Session ID: extend the regex to match possible hash representations
10c78d1 
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

TODO:
 - remove `uniqid()` usage

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 4, 2015
@virtualtam
Session ID: extend the regex to match possible hash representations
7bb89f8 
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - `index.php`:
   - remove `uniqid()` usage
   - call `session_regenerate_id()` if an invalid cookie is detected
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 4, 2015
@virtualtam
Session ID: extend the regex to match possible hash representations
60a3976 
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - `index.php`:
   - remove `uniqid()` usage
   - call `session_regenerate_id()` if an invalid cookie is detected
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Sep 6, 2015
@virtualtam
Session ID: extend the regex to match possible hash representations
68bc213 
Improves shaarli#306
Relates to shaarli#335 & shaarli#336
Duplicated by shaarli#339

Issues:
 - PHP regenerates the session ID if it is not compliant
 - the regex checking the session ID does not cover all cases
   - different algorithms: md5, sha1, sha256, etc.
   - bit representations: 4, 5, 6

Fix:
 - `index.php`:
   - remove `uniqid()` usage
   - call `session_regenerate_id()` if an invalid cookie is detected
 - regex: support all possible characters - '[a-zA-Z,-]{2,128}'
 - tests: add coverage for all algorithms & bit representations

See:
 - http://php.net/manual/en/session.configuration.php#ini.session.hash-function
 - https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
 - http://php.net/manual/en/function.session-id.php
 - http://php.net/manual/en/function.session-regenerate-id.php
 - http://php.net/manual/en/function.hash-algos.php

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug it's broken! security
Projects
None yet
Milestone
0.5.3
Development

Successfully merging this pull request may close these issues.
3 participants
@ArthurHoaro @virtualtam @nodiscc