-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Restore CLI precedence for web headers and HTTPS over web_dev_config.yaml #179639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this 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 adjusts the configuration loading logic to ensure that command-line arguments have precedence over settings in web_dev_config.yaml. The changes affect how HTTPS configuration and web server headers are processed in the drive and run commands. Specifically, the order of merging headers is corrected in WebDevServerConfig.copyWith, and the logic for determining HttpsConfig is updated in both DriveCommand and RunCommand. While the changes are mostly correct, I've identified a logical flaw in drive.dart where CLI arguments for HTTPS are incorrectly ignored if a configuration file is not present, and I've provided a suggestion to align it with the correct implementation in run.dart.
|
Thanks for the review! I've updated the logic to match run.dart so that: CLI arguments always take precedence File config works only as a fallback HTTPS config is correctly applied even without web_dev_config.yaml Please let me know if anything else is needed. |
bkonyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Saqib198!
This looks good to me in general, with a couple of minor suggestions. Also, this will need a test before it can be submitted.
| // Determine HTTPS config with CLI > file precedence | ||
| // If CLI provides TLS certs, use them (with file config as fallback for missing values) | ||
| // If CLI doesn't provide TLS certs, use file config | ||
| final HttpsConfig? httpsConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider doing this instead (also in drive.dart):
final HttpsConfig? httpsConfig = (fileConfig.https ?? HttpsConfig()).copyWith(
certPath: stringArg('web-tls-cert-path'),
certKeyPath: stringArg('web-tls-cert-key-path'),
);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @bkonyi!
I tried implementing it, but HttpsConfig() cannot be instantiated without arguments since both certPath and certKeyPath are required parameters in the constructor:
const HttpsConfig({required this.certPath, required this.certKeyPath});
Instead, I've simplified the code using HttpsConfig.parse() which handles all the cases cleanly:
final HttpsConfig? httpsConfig = HttpsConfig.parse(
stringArg('web-tls-cert-path') ?? fileConfig.https?.certPath,
stringArg('web-tls-cert-key-path') ?? fileConfig.https?.certKeyPath,
);
This achieves the same simplification (reduced from 15 lines to 4 lines) while:
- CLI args take precedence over file config
- Falls back to file config values when CLI args not provided
- Returns null if neither provides both values
- Throws ArgumentError if only one path is provided (existing behavior)
I've also added tests for CLI precedence as requested. Let me know if you'd like any changes!
4521acd to
5f030a9
Compare
| } | ||
| } | ||
|
|
||
| Future<WebDevServerConfig> webDevServerConfigCore() async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Saqib198 – I moved things into this shared function so we have fewer places to keep this logic up-to-date!
|
I took over a bit to fix the analyzer and format issue and to do a tiny bit of cleanup |
|
Sorry for the delay here! Help me understand. The way I'm seeing this PR, it seems to fix the precedence for |
| List<ProxyRule>? proxy, | ||
| }) { | ||
| return WebDevServerConfig( | ||
| host: host ?? this.host, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the way they are now is correct. But the issue says web-port's and web-hostname's precedence is incorrect, which is confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah!
I get it.
By putting headers after this.headers, it overwrites.
But with port and host, the ?? has things the other way.
So it reads weird, but I think the code as it is now is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I agree, I see no code changes for anything but headers.
So was it really broken? Or are we missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out — happy to clarify.
You’re right that web-port and web-hostname already had correct CLI > file precedence.
They use the pattern:
host: host ?? this.host,
port: port ?? this.port,
where host / port come from the CLI layer, so CLI values correctly override
web_dev_config.yaml today.
The regression introduced in 3.38 specifically affected:
• headers (due to merge order: {...?headers, ...this.headers})
• HTTPS config (due to conditional logic that ignored CLI TLS args when file config
was present or missing)
That’s why this PR only changes:
• header merge order
• HTTPS config resolution
The issue description mentioned web-port / web-hostname because the symptom users
observed was “CLI args ignored”, but the actual root cause was headers + HTTPS.
Port/hostname were already behaving correctly, so no change was needed there.
Happy to update the PR description or issue wording if that helps reduce confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes more sense. Thanks for clarifying!
I would say yes let's update the issue and PR descriptions. If it confused me and @kevmoo, I'm sure it'll confuse more people later when they look at the history of this code.
mdebbar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@bkonyi does this look good to you? I think we need a 2nd approval to merge. |
|
autosubmit label was removed for flutter/flutter/179639, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
|
autosubmit label was removed for flutter/flutter/179639, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Roll Flutter from 13b2b91 to 01d37bc (74 revisions) flutter/flutter@13b2b91...01d37bc 2026-01-07 stuartmorgan@google.com Replace Hybrid Composition wiki page with dev-facing content (flutter/flutter#180642) 2026-01-07 116356835+AbdeMohlbi@users.noreply.github.com Improve code quality in `FlutterActivityTest.java` (flutter/flutter#180585) 2026-01-07 iinozemtsev@google.com Roll Dart SDK to 3.11.0-296.1.beta (flutter/flutter#180633) 2026-01-07 vegorov@google.com Bump target Windows version to 10 (flutter/flutter#180624) 2026-01-07 goderbauer@google.com Run hook_user_defines and link_hook integration tests on CI (flutter/flutter#180622) 2026-01-07 iliyazelenkog@gmail.com Fix division by zero in RenderTable intrinsic size methods (flutter/flutter#178217) 2026-01-07 116356835+AbdeMohlbi@users.noreply.github.com Remove more `requires 24` anotations (flutter/flutter#180116) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from 3971dbb85d45 to c5359a4d720e (1 revision) (flutter/flutter#180631) 2026-01-07 huy@nevercode.io Fix TabBar.image does not render at initialIndex for the first time (flutter/flutter#179616) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from 1e7ad625f6f7 to 3971dbb85d45 (3 revisions) (flutter/flutter#180627) 2026-01-07 goderbauer@google.com Unpin DDS (flutter/flutter#180571) 2026-01-07 bruno.leroux@gmail.com Fix DropdownMenuEntry.style not resolved when entry is highlighted (flutter/flutter#178873) 2026-01-07 116356835+AbdeMohlbi@users.noreply.github.com Remove unnecessary `@RequiresApi24` annotations from FlutterFragment methods (flutter/flutter#180117) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from 7fc63228056b to 1e7ad625f6f7 (1 revision) (flutter/flutter#180616) 2026-01-07 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from QfR2ZFZ5kGTD3raO3... to dTvN_JVSCfGFRasvH... (flutter/flutter#180612) 2026-01-07 bkonyi@google.com [ Widget Preview ] Add support for `dart:ffi` imports (flutter/flutter#180586) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from eec90000a899 to 7fc63228056b (2 revisions) (flutter/flutter#180608) 2026-01-07 ulisses.hen@hotmail.com Add --web-define flag for template variable injection in Flutter web builds (flutter/flutter#175805) 2026-01-07 codefu@google.com docs(engine): update rbe notes (flutter/flutter#180599) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from b6249496c230 to eec90000a899 (3 revisions) (flutter/flutter#180602) 2026-01-06 146823759+brahim-guaali@users.noreply.github.com Forward proxy 404 responses to client (flutter/flutter#179858) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 40a8c6188f7f to d2e3ce177270 (1 revision) (flutter/flutter#180598) 2026-01-06 104009581+Saqib198@users.noreply.github.com Restore CLI precedence for web headers and HTTPS over web_dev_config.yaml (flutter/flutter#179639) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from f5d9da13d56d to b6249496c230 (1 revision) (flutter/flutter#180596) 2026-01-06 dkwingsmt@users.noreply.github.com Enable misc leak tracking (flutter/flutter#176992) 2026-01-06 dacoharkes@google.com [hooks] Don't require NDK for Android targets (flutter/flutter#180594) 2026-01-06 kjlubick@users.noreply.github.com [skia] Disable legacy non-const SkData APIs (flutter/flutter#179684) 2026-01-06 48625061+muradhossin@users.noreply.github.com Fix/ios share context menu (flutter/flutter#176199) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from b970aeffa66f to f5d9da13d56d (5 revisions) (flutter/flutter#180588) 2026-01-06 goderbauer@google.com Manually bump dependencies (flutter/flutter#180509) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 8150be8a0e48 to 40a8c6188f7f (2 revisions) (flutter/flutter#180582) 2026-01-06 engine-flutter-autoroll@skia.org Roll Packages from 12eb764 to d3f860d (2 revisions) (flutter/flutter#180581) 2026-01-06 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from MHF-UAfO6sVKqSEYk... to nR2ESa1Gd8yPcWo06... (flutter/flutter#180578) 2026-01-06 sokolovskyi.konstantin@gmail.com Add tooltip support to PlatformMenuItem and PlatformMenu. (flutter/flutter#180069) 2026-01-06 bruno.leroux@gmail.com Add DropdownMenuFormField.errorBuilder (flutter/flutter#179345) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from 1845397e11ba to b970aeffa66f (2 revisions) (flutter/flutter#180566) 2026-01-06 oss@simonbinder.eu Don't embed unreferenced assets (flutter/flutter#179251) 2026-01-06 116356835+AbdeMohlbi@users.noreply.github.com Improve documentation about ValueNotifier's behavior (flutter/flutter#179870) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from 904ba00331ca to 1845397e11ba (5 revisions) (flutter/flutter#180558) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 2fb9ad834c4d to 8150be8a0e48 (1 revision) (flutter/flutter#180557) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from 98c042dde68c to 904ba00331ca (3 revisions) (flutter/flutter#180550) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from ba9f7f790966 to 2fb9ad834c4d (2 revisions) (flutter/flutter#180548) 2026-01-06 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ubBGcRaAKWKihQ4ac... to QfR2ZFZ5kGTD3raO3... (flutter/flutter#180547) 2026-01-06 ahmedsameha1@gmail.com Make sure that a DraggableScrollableSheet doesn't crash in 0x0 enviro… (flutter/flutter#180433) 2026-01-06 ahmedsameha1@gmail.com Make sure that a ColorFiltered doesn't crash 0x0 environment (flutter/flutter#180307) 2026-01-06 ahmedsameha1@gmail.com Make sure that a FadeInImage doesn't crash in 0x0 environment (flutter/flutter#180495) ...

This PR restores the correct precedence behavior for web development server configuration. Since Flutter 3.38, CLI arguments were being ignored when
web_dev_config.yamlwas present — specifically for HTTPS and headers.According to the design and documentation, the precedence should be:
web_dev_config.yamlRoot Cause
The regression affected:
Note:
--web-portand--web-hostnamewere already working correctly due to thehost ?? this.hostandport ?? this.portpattern.The observed issue was due to headers + HTTPS only.
Changes Made
runanddrivecommandsHttpsConfig.parse()Before (broken)
❌ CLI TLS args ignored when config file existed
❌ headers overridden by file config
After (fixed)
✅ CLI TLS args respected
✅ headers override config as expected
Tests
Fixes
Fixes: #179014