Skip to content

fix: validate devtoolsOptionsUri in extensionEnabledState handler#9834

Open
adilburaksen wants to merge 3 commits into
flutter:masterfrom
adilburaksen:fix/extension-enabled-state-path-validation
Open

fix: validate devtoolsOptionsUri in extensionEnabledState handler#9834
adilburaksen wants to merge 3 commits into
flutter:masterfrom
adilburaksen:fix/extension-enabled-state-path-validation

Conversation

@adilburaksen
Copy link
Copy Markdown

Summary

handleExtensionEnabledState() in _devtools_extensions.dart reads devtoolsOptionsUri directly from query parameters and passes it without validation to DevToolsOptions.setExtensionEnabledState(), which creates the target file and any missing parent directories.

// Before fix — no URI validation
final devtoolsOptionsFileUri = Uri.parse(devtoolsOptionsFileUriString);
// → passes directly to File.fromUri(devtoolsOptionsFileUri) in extension_enablement.dart

An untrusted caller that can reach the DevTools server (e.g., another local process, or a browser tab via cross-origin requests) can create or overwrite any file writable by the server process:

GET /api/extensionEnabledState?
    devtoolsOptionsUri=file:///home/user/.ssh/authorized_keys
    &extensionName=pwned&enabled=true

The resulting file content is a valid YAML map:

extensions:
  pwned: enabled

Fix

Reject requests where devtoolsOptionsUri:

  • does not use the file: scheme, or
  • does not end with devtools_options.yaml

Both conditions are required for a legitimate DevTools options file. Any other value is rejected with HTTP 400.

Impact

Without the fix, any local process or browser tab able to reach the DevTools HTTP server can create or overwrite arbitrary files on the developer's machine with attacker-controlled YAML content.

…ledState

handleExtensionEnabledState() accepted the devtoolsOptionsUri query
parameter without validation and passed it directly to
DevToolsOptions.setExtensionEnabledState(), which creates the file and
parent directories if they do not exist. An untrusted caller could supply
an arbitrary file: URI to create or overwrite any path writable by the
DevTools server process.

Add a guard that rejects URIs whose scheme is not 'file' or whose path
does not end with 'devtools_options.yaml'. Both conditions must hold for
a legitimate devtools options file.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds validation for the devtoolsOptionsUri query parameter to ensure it is a local file URI pointing to a devtools_options.yaml file, mitigating potential file overwrite vulnerabilities. Feedback was provided to tighten the path validation logic, as the current implementation could match unintended filenames.

Comment thread packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart Outdated
endsWith('devtools_options.yaml') matched paths like
'not_devtools_options.yaml'. Adding the path separator prefix
ensures only files literally named 'devtools_options.yaml' are accepted.
@adilburaksen
Copy link
Copy Markdown
Author

Friendly nudge — open ~18 days. Validates devtoolsOptionsUri in extensionEnabled to prevent the file-write primitive. Happy to address any feedback or add coverage if that helps move it along.

// string would allow an untrusted caller to create or overwrite any file
// writable by the DevTools server process.
if (devtoolsOptionsFileUri.scheme != 'file' ||
!devtoolsOptionsFileUri.path.endsWith('/devtools_options.yaml')) {
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.

will this forward slash work with windows file schemes?

Copy link
Copy Markdown
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

Please also add an entry to the devtools_shared changelog.

Resolve the file name via Uri.toFilePath() + p.basename instead of a
forward-slash string match so the check holds for Windows file URIs
(both '/' and '\' separators) and reject non-local (UNC) authorities.
Bump devtools_shared to 13.0.2 and add a changelog entry.
@adilburaksen
Copy link
Copy Markdown
Author

Thanks for the review. Addressed both points in the latest commit:

  • Windows file schemes: replaced the path.endsWith('/devtools_options.yaml') check with p.basename(uri.toFilePath()). toFilePath() resolves to the host's native path so the file-name match works for both / and \ separators, which fixes the forward-slash concern. I also require an empty URI host, which rejects UNC paths (file://server/share/...) and keeps toFilePath() from throwing on a non-local authority.
  • Changelog: added an entry to packages/devtools_shared/CHANGELOG.md and bumped the package version to 13.0.2.

The existing apiExtensionEnabledState tests build the URI via Uri.file(...).toString(), so they still pass the new validation (file scheme, empty host, exact file name).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants