Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented May 30, 2018

If a PUT request comes in that is not JSON or from encoded. Then we can
only read it (exactly) once. If that is the case we must assume no
shared secret is set.

If we don't then we either are the first to read it, thus causing the
real read of the data to fail.

Or we are later and then it throws an exception (also failing the
request).

To trigger you need quite a few conditions.

  1. configure are shared_secret logcondition in your config.php
  2. Have an app installed that on PUT somewhere logs to debug
  3. Upload a file to a public share (was the easiest to trigger).

On our instance this was the files_antivirus app.

This write to debug will try to read the log_secret from the request. But this will fail hard. Causing the request to fail.

If a PUT request comes in that is not JSON or from encoded. Then we can
only read it (exactly) once. If that is the case we must assume no
shared secret is set.

If we don't then we either are the first to read it, thus causing the
real read of the data to fail.

Or we are later and then it throws an exception (also failing the
request).

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the bugfix/noid/logsecret_fix_file_put branch from 9afa674 to a52d206 Compare May 30, 2018 18:18
@codecov
Copy link

codecov bot commented May 30, 2018

Codecov Report

Merging #9692 into master will decrease coverage by 45.1%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master   #9692       +/-   ##
============================================
- Coverage     51.73%   6.63%   -45.11%     
- Complexity    25710   25713        +3     
============================================
  Files          1635    1635               
  Lines         95965   95970        +5     
  Branches       1384    1384               
============================================
- Hits          49650    6363    -43287     
- Misses        46315   89607    +43292
Impacted Files Coverage Δ Complexity Δ
lib/private/Log.php 0% <0%> (-73.08%) 38 <0> (+3)
core/Command/Background/WebCron.php 0% <0%> (-100%) 1% <0%> (ø)
apps/dav/lib/Db/Direct.php 0% <0%> (-100%) 1% <0%> (ø)
...eLimiting/Exception/RateLimitExceededException.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/AppFramework/Utility/TimeFactory.php 0% <0%> (-100%) 1% <0%> (ø)
...te/Authentication/LoginCredentials/Credentials.php 0% <0%> (-100%) 4% <0%> (ø)
...b/public/AppFramework/Db/DoesNotExistException.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/Federation/CloudId.php 0% <0%> (-100%) 5% <0%> (ø)
lib/private/Comments/ManagerFactory.php 0% <0%> (-100%) 2% <0%> (ø)
lib/private/Accounts/Hooks.php 0% <0%> (-100%) 14% <0%> (ø)
... and 880 more

@MorrisJobke
Copy link
Member

@danxuliu Mind to have a look at the broken integration test :/ Seem that something slipped through :/

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code makes sense 👍

@MorrisJobke
Copy link
Member

@danxuliu Mind to have a look at the broken integration test :/ Seem that something slipped through :/

It's the same on master and stable12/13

@MorrisJobke MorrisJobke merged commit f13c2b2 into master Jun 6, 2018
@MorrisJobke MorrisJobke deleted the bugfix/noid/logsecret_fix_file_put branch June 6, 2018 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants