test: add test to reproduce https://github.com/ory/hydra/issues/1719 #1734
Conversation
aaslamin
force-pushed
the
oauth2/reproduce-1719
branch
from
de933f5 to
6b4a79e
Compare
| stopWorkers() | ||
| successCh <- struct{}{} | ||
| return | ||
| } |
There was a problem hiding this comment.
This is where you want to end up in! (to reproduce the 🐛)
| // race again! | ||
| postgresRegistry.OAuth2Storage().CreateRefreshTokenSession(ctx, tokenSignature, request) | ||
| stopWorkers() | ||
| } |
|
Yo, this is a sweet test case! If you'd be up for fixing the issue as well I think this can be merged right away! |
@aeneasr I'll take a look at this later in the week when I get a free cycle. However, I think the solution lies in We may need to handle this on a DB by DB basis as the error copy returned by Postgres, MySQL and CockroachDB could be different when an outdated read is done in a transaction utilizing the repeatable read iso level. Alternatively, we could return the raw error from fosite and let the Hydra deal with it via the sqlx package. |
|
You're the best man! I think we can generally enforce one transaction type for all databases, and release a beta to see if it has negative performance impacts for people. What do you think? |
|
|
aaslamin
force-pushed
the
oauth2/reproduce-1719
branch
from
6b4a79e to
5b485db
Compare
|
Closing in favour of #1766 which has a better implementation. |
Related issue
#1719
What's going on here...
This PR adds a test that consistently reproduces the bug identified in the linked issue. To run locally, navigate to the
oauth2directory and run:Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.