镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Improve networking checks - #26266

Merged
rullzer merged 4 commits into
masterfrom
future-proof-networking
Apr 12, 2021
Merged

rullzer merged 4 commits into
masterfrom
future-proof-networking

Conversation

@LukasReschke

@LukasReschke LukasReschke commented Mar 23, 2021 •

Copy link
Copy Markdown
Member

Whilst we currently state that SSRF is generally outside of our threat model, this is something where we should invest to improve this.

Todos:

  • Make sure existing tests pass
  • Investigate how far tests can be added
  • Rebase and proper commit messages
  • phpcs

Comment thread lib/private/Http/Client/Client.php Resolved
@LukasReschke
LukasReschke force-pushed the future-proof-networking branch from dbc1f25 to b35feec Compare March 23, 2021 17:23
@LukasReschke
LukasReschke force-pushed the future-proof-networking branch 2 times, most recently from e545c19 to e0421fa Compare March 30, 2021 18:00
@LukasReschke
LukasReschke force-pushed the future-proof-networking branch from 5d9189f to 2baaa6f Compare March 30, 2021 18:07
@LukasReschke LukasReschke changed the title [WIP] Improve networking checks Improve networking checks Mar 30, 2021
@LukasReschke
LukasReschke marked this pull request as ready for review March 30, 2021 18:10
@LukasReschke LukasReschke added 3. to review Waiting for reviews security labels Mar 30, 2021
@LukasReschke

Copy link
Copy Markdown
Member Author

Couldn't test the app store page as it seems offline currently:

{"reqId":"MgQTR6OISoKtHPl9NqCw","level":2,"time":"2021-03-30T17:57:31+00:00","remoteAddr":"172.18.0.1","user":"--","app":"appstoreFetcher","method":"POST","url":"/index.php","message":"Could not connect to appstore: cURL error 7:  (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for https://apps.nextcloud.com/api/v1/apps.json","userAgent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.3 Safari/605.1.15","version":"22.0.0.2"}

@LukasReschke

Copy link
Copy Markdown
Member Author

154	There were 2 errors:
155	
156	1) Test\Http\Client\NegativeDnsCacheTest::testSetNegativeCacheForDnsType
157	Error: Class 'Test\Http\Client\NegativeDnsCache' not found
158	
159	/drone/src/tests/lib/Http/Client/NegativeDnsCacheTest.php:50
160	
161	2) Test\Http\Client\NegativeDnsCacheTest::testIsNegativeCached
162	Error: Class 'Test\Http\Client\NegativeDnsCache' not found
163	
164	/drone/src/tests/lib/Http/Client/NegativeDnsCacheTest.php:50
165	
166	

😢

@MorrisJobke MorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Beside my nitpicks it looks good 👍

@MorrisJobke MorrisJobke added this to the Nextcloud 22 milestone Apr 1, 2021
Whilst we currently state that SSRF is generally outside of our threat model, this is something where we should invest to improve this.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke
LukasReschke force-pushed the future-proof-networking branch from 38ad8dd to d5c7bb2 Compare April 6, 2021 11:38
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke

LukasReschke commented Apr 6, 2021 •

Copy link
Copy Markdown
Member Author

Rebased and added the suggestions by Morris. (let's hope the tests still pass 😉 )

@rullzer @ChristophWurst - mind giving this a review as well? This will only land in master, so we can iron out any potential bugs before a release :)

@rullzer
rullzer merged commit 789bb0d into master Apr 12, 2021
@rullzer
rullzer deleted the future-proof-networking branch April 12, 2021 10:42
$this->logger = $logger;
}

public function ThrowIfLocalIp(string $ip) : void {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be throwIfLocalIp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sk1p

sk1p commented Jun 7, 2022

Copy link
Copy Markdown

Just a quick ping that this PR possibly (probably?) caused a regression for streaming from external storage, please see #30282

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 security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants