{{ message }}
Repository navigation
Conversation
…ardening Seven separate issues found during a source review: - FileController::link() required only read permission (the default permissionFind threshold) to toggle a file's public guest_access flag, letting a read-only recipient publish a share link. Now requires write permission. - UserBot::shouldDispatchWebhook() validated the configured webhook_url with a scheme regex only, no check against private/loopback/ link-local destinations, so a bot owner could point the webhook at an internal service and receive the bot's own token plus a short-lived user token delivered to it. Added Ihttp::isPublicDestination(), which resolves every A/AAAA record for the host and rejects any private/ reserved/loopback/link-local result, including IPv4-mapped/6to4/ NAT64-encoded private addresses embedded in an IPv6 literal. - WebSocketService::onOpen()'s 'web' connection path only matched the user row, unlike the HTTP auth path (User::authInfo()) which also calls UserDevice::check() to reject a revoked device token. Added the same check to the WebSocket path. - WebSocketDialogMsg fetched attacker-supplied image URLs from chat message markup via file_get_contents() with no destination restriction. Guarded both call sites with the same Ihttp::isPublicDestination() check. - FileController::office__token() never called User::auth(), unlike every sibling action in the controller, letting any unauthenticated caller get an arbitrary config payload JWT-signed with app.key. Added the missing check. - Ihttp::ihttp_request() disabled CURLOPT_SSL_VERIFYPEER/ CURLOPT_SSL_VERIFYHOST and pinned every outbound connection to legacy TLS 1.0, exposing AI-provider keys/responses and license data to network-level interception. Enabled verification and let curl negotiate its own default protocol. - The QR login exchange read its approval state via Cache::get() without consuming it, so an approved QR code remained redeemable by anyone holding its key for the rest of the cache TTL. Changed to Cache::pull() for atomic get-and-delete. Fixes kuaifan#312.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #312
Summary
Seven separate hardening fixes from a source review:
FileController::link()now requires write permission (File::permissionFind($id, $user, 1)) to toggle a file's publicguest_accessflag, instead of the default read-only threshold.UserBot::shouldDispatchWebhook()now rejects webhook URLs that resolve to a private, loopback, link-local, or otherwise reserved destination via a newIhttp::isPublicDestination()helper.WebSocketService::onOpen()'s'web'connection path now also callsUserDevice::check(), matching the HTTP auth path, so a revoked device token can no longer open a new WebSocket connection.WebSocketDialogMsg's two chat-image-fetch call sites now useIhttp::isPublicDestination()before fetching an attacker-supplied image URL from message markup.FileController::office__token()now callsUser::auth(), matching every sibling action in the controller.Ihttp::ihttp_request()now enables TLS certificate/hostname verification and no longer pins outbound connections to TLS 1.0.Cache::pull($key)(atomic get-and-delete) instead ofCache::get($key), so an approved QR code can only be redeemed once.New helper
Ihttp::isPublicDestination($url)/Ihttp::isPublicIp($ip): resolves every A/AAAA record for the URL's host and rejects if any result falls in a private/reserved/loopback/link-local range, including IPv4-mapped, 6to4, and NAT64-encoded private addresses embedded in an IPv6 literal (filter_var's ownFILTER_FLAG_NO_PRIV_RANGE/FILTER_FLAG_NO_RES_RANGEflags do not catch these encodings on their own).Testing
This environment has no composer/vendor install or Docker environment available (the same constraint already noted in #308's own PR). Verified via
php -lon all six changed files (no syntax errors), manual comparison against each fix's correctly-behaving sibling call site already present elsewhere in the same files, and an independent standalone re-implementation ofisPublicDestination/isPublicIp's logic exercised against 13 cases (public IP, loopback, RFC1918, link-local/cloud-metadata address, non-http(s) schemes, and the three IPv6-encoding bypass classes), all 13 behaving as expected.