Skip to content

Commit 74e64d9

Browse files
LutzLangeclaude
andcommitted
fix: read browser policy the way Chrome and Firefox do
- Chromium policy files may contain comments and trailing commas, as Chromium's loader accepts them (JSON_PARSE_CHROMIUM_EXTENSIONS | JSON_ALLOW_TRAILING_COMMAS); the Firefox reader stays strict JSON - a conflict between legacy ProxyMode/ProxyPacUrl values no longer hides a ProxySettings dictionary, which overrides them - Linux Firefox: only the file Firefox uses counts (/etc/firefox/policies/policies.json when it exists, otherwise the install's distribution/ file), so a Proxy section in an ignored file can't be reported Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent b031324 commit 74e64d9

1 file changed

Lines changed: 208 additions & 26 deletions

File tree

‎crates/agent/src/browser_pac.rs‎

Lines changed: 208 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,92 @@ fn firefox_proxy(policies: &Value) -> Option<(Option<String>, bool)> {
241241
Some((pac_url, locked))
242242
}
243243

244+
/// Chromium policy JSON: Chromium's loader accepts comments and trailing
245+
/// commas (`JSON_PARSE_CHROMIUM_EXTENSIONS | JSON_ALLOW_TRAILING_COMMAS`).
246+
#[cfg(any(target_os = "linux", test))]
247+
fn read_chromium_json(file: &Path) -> Result<Value, String> {
248+
let contents = std::fs::read_to_string(file)
249+
.map_err(|error| format!("read {}: {error}", file.display()))?;
250+
serde_json::from_str(&strip_jsonc(&contents))
251+
.map_err(|error| format!("parse {}: {error}", file.display()))
252+
}
253+
254+
/// Removes `//` and `/* */` comments, then trailing commas, outside
255+
/// strings, in two scans; a comma looks ahead only past whitespace.
256+
#[cfg(any(target_os = "linux", test))]
257+
fn strip_jsonc(input: &str) -> String {
258+
let mut uncommented = String::with_capacity(input.len());
259+
let mut chars = input.chars().peekable();
260+
let mut in_string = false;
261+
while let Some(c) = chars.next() {
262+
if in_string {
263+
uncommented.push(c);
264+
match c {
265+
'\\' => uncommented.extend(chars.next()),
266+
'"' => in_string = false,
267+
_ => {}
268+
}
269+
continue;
270+
}
271+
match (c, chars.peek()) {
272+
('/', Some('/')) => {
273+
for next in chars.by_ref() {
274+
if next == '\n' {
275+
uncommented.push('\n');
276+
break;
277+
}
278+
}
279+
}
280+
('/', Some('*')) => {
281+
chars.next();
282+
let mut previous = '\0';
283+
for next in chars.by_ref() {
284+
if previous == '*' && next == '/' {
285+
break;
286+
}
287+
previous = next;
288+
}
289+
uncommented.push(' ');
290+
}
291+
_ => {
292+
in_string = c == '"';
293+
uncommented.push(c);
294+
}
295+
}
296+
}
297+
298+
let chars: Vec<char> = uncommented.chars().collect();
299+
let mut out = String::with_capacity(uncommented.len());
300+
let mut in_string = false;
301+
let mut index = 0;
302+
while index < chars.len() {
303+
let c = chars[index];
304+
if in_string {
305+
out.push(c);
306+
if c == '\\' {
307+
if let Some(next) = chars.get(index + 1) {
308+
out.push(*next);
309+
index += 1;
310+
}
311+
} else if c == '"' {
312+
in_string = false;
313+
}
314+
} else if c == ','
315+
&& matches!(
316+
chars[index + 1..].iter().find(|next| !next.is_whitespace()),
317+
Some('}' | ']')
318+
)
319+
{
320+
// trailing comma: dropped
321+
} else {
322+
in_string = c == '"';
323+
out.push(c);
324+
}
325+
index += 1;
326+
}
327+
out
328+
}
329+
244330
#[cfg(any(target_os = "linux", windows, test))]
245331
fn read_json(file: &Path) -> Result<Value, String> {
246332
let contents = std::fs::read_to_string(file)
@@ -292,10 +378,10 @@ mod linux {
292378
("opt/firefox/firefox", "system"),
293379
];
294380

295-
/// Firefox policy files, system-wide first (read by the deb and the
296-
/// snap), then the install's `distribution/`. The first file that sets
297-
/// `Proxy` is reported; Firefox's own precedence between the two is not
298-
/// verified here.
381+
/// Firefox policy files in Firefox's own order: it uses
382+
/// `/etc/firefox/policies/policies.json` when that file exists (read by
383+
/// the deb and the snap) and otherwise the install's `distribution/`
384+
/// file, never both.
299385
const FIREFOX_POLICY_FILES: &[&str] = &[
300386
"etc/firefox/policies/policies.json",
301387
"usr/lib/firefox/distribution/policies.json",
@@ -400,6 +486,7 @@ mod linux {
400486
) -> Result<Option<(ProxyPolicy, PathBuf)>, String> {
401487
const KEYS: [&str; 3] = ["ProxySettings", "ProxyMode", "ProxyPacUrl"];
402488
let mut merged: Vec<(&str, Value, PathBuf)> = Vec::new();
489+
let mut conflicts: Vec<(&str, PathBuf, PathBuf)> = Vec::new();
403490
for directory in directories {
404491
let entries = match std::fs::read_dir(directory) {
405492
Ok(entries) => entries,
@@ -419,7 +506,7 @@ mod linux {
419506
.collect();
420507
files.sort();
421508
for file in files {
422-
let value = match read_json(&file) {
509+
let value = match read_chromium_json(&file) {
423510
Ok(value) => value,
424511
Err(error) => {
425512
unreadable.push(error);
@@ -432,18 +519,28 @@ mod linux {
432519
};
433520
match merged.iter().find(|(existing, _, _)| *existing == key) {
434521
Some((_, existing, first)) if existing != setting => {
435-
return Err(format!(
436-
"conflicting proxy policy in {} and {}",
437-
first.display(),
438-
file.display()
439-
));
522+
conflicts.push((key, first.clone(), file.clone()));
440523
}
441524
Some(_) => {}
442525
None => merged.push((key, setting.clone(), file.clone())),
443526
}
444527
}
445528
}
446529
}
530+
// ProxySettings overrides the legacy keys, so their conflicts only
531+
// matter when it isn't set (or conflicts itself).
532+
let settings_in_effect = merged.iter().any(|(key, _, _)| *key == "ProxySettings")
533+
&& !conflicts.iter().any(|(key, _, _)| *key == "ProxySettings");
534+
if let Some((_, first, second)) = conflicts
535+
.iter()
536+
.find(|(key, _, _)| !settings_in_effect || *key == "ProxySettings")
537+
{
538+
return Err(format!(
539+
"conflicting proxy policy in {} and {}",
540+
first.display(),
541+
second.display()
542+
));
543+
}
447544
// The source shown is the file that set the PAC URL, if any.
448545
let Some(source) = ["ProxySettings", "ProxyPacUrl", "ProxyMode"]
449546
.iter()
@@ -469,26 +566,27 @@ mod linux {
469566
..BrowserPac::default()
470567
};
471568
let mut found_policy = false;
472-
for file in FIREFOX_POLICY_FILES.iter().map(|file| root.join(file)) {
473-
if !file.is_file() {
474-
continue;
475-
}
476-
let value = match read_json(&file) {
477-
Ok(value) => value,
569+
// Only the file Firefox uses; a Proxy section in a file it ignores
570+
// must not count.
571+
if let Some(file) = FIREFOX_POLICY_FILES
572+
.iter()
573+
.map(|file| root.join(file))
574+
.find(|file| file.is_file())
575+
{
576+
match read_json(&file) {
577+
Ok(value) => {
578+
if let Some((pac_url, locked)) = firefox_proxy(&value) {
579+
row.pac_url = pac_url;
580+
row.locked = locked;
581+
row.source = Some(file);
582+
found_policy = true;
583+
}
584+
}
478585
Err(detail) => {
479586
row.detail = Some(crate::reconcile::truncate_detail(detail));
480587
found_policy = true;
481-
break;
482588
}
483-
};
484-
let Some((pac_url, locked)) = firefox_proxy(&value) else {
485-
continue;
486-
};
487-
row.pac_url = pac_url;
488-
row.locked = locked;
489-
row.source = Some(file);
490-
found_policy = true;
491-
break;
589+
}
492590
}
493591
(packaging.is_some() || found_policy).then_some(row)
494592
}
@@ -653,6 +751,90 @@ mod linux {
653751
fs::remove_dir_all(root).unwrap();
654752
}
655753

754+
#[test]
755+
fn chromium_policy_files_may_have_comments_and_trailing_commas() {
756+
let root = temp_root("jsonc");
757+
write(
758+
&root,
759+
"etc/opt/chrome/policies/managed/egress.json",
760+
&format!(
761+
"// AI egress\n{{\n /* PAC */ \"ProxySettings\": {{\"ProxyMode\": \"pac_script\", \"ProxyPacUrl\": \"{PAC}\",}},\n}}\n"
762+
),
763+
);
764+
assert_eq!(discover(&root)[0].pac_url.as_deref(), Some(PAC));
765+
fs::remove_dir_all(root).unwrap();
766+
}
767+
768+
#[test]
769+
fn strip_jsonc_keeps_strings_intact() {
770+
assert_eq!(
771+
strip_jsonc(
772+
r#"{"u": "http://a/b,}", /* c */ "v": [1, 2,], // x
773+
}"#
774+
),
775+
"{\"u\": \"http://a/b,}\", \"v\": [1, 2] \n}"
776+
);
777+
assert_eq!(strip_jsonc(r#"{"q": "a\"//b"}"#), r#"{"q": "a\"//b"}"#);
778+
}
779+
780+
#[test]
781+
fn proxy_settings_wins_over_conflicting_legacy_keys() {
782+
let root = temp_root("precedence");
783+
write(
784+
&root,
785+
"etc/opt/chrome/policies/managed/a.json",
786+
r#"{"ProxyMode":"pac_script","ProxyPacUrl":"https://old.example/a.pac"}"#,
787+
);
788+
write(
789+
&root,
790+
"etc/opt/chrome/policies/managed/b.json",
791+
r#"{"ProxyPacUrl":"https://old.example/b.pac"}"#,
792+
);
793+
write(
794+
&root,
795+
"etc/opt/chrome/policies/managed/c.json",
796+
&format!(
797+
r#"{{"ProxySettings":{{"ProxyMode":"pac_script","ProxyPacUrl":"{PAC}"}}}}"#
798+
),
799+
);
800+
let chrome = &discover(&root)[0];
801+
assert_eq!(chrome.pac_url.as_deref(), Some(PAC));
802+
assert_eq!(chrome.detail, None);
803+
fs::remove_dir_all(root).unwrap();
804+
}
805+
806+
#[test]
807+
fn firefox_reads_only_the_file_it_uses() {
808+
let root = temp_root("ffactive");
809+
write(&root, "usr/lib/firefox/firefox", "");
810+
// /etc exists without Proxy: the distribution file is ignored.
811+
write(
812+
&root,
813+
"etc/firefox/policies/policies.json",
814+
r#"{"policies":{"DisableAppUpdate":true}}"#,
815+
);
816+
write(
817+
&root,
818+
"usr/lib/firefox/distribution/policies.json",
819+
&format!(
820+
r#"{{"policies":{{"Proxy":{{"Mode":"autoConfig","AutoConfigURL":"{PAC}","Locked":true}}}}}}"#
821+
),
822+
);
823+
let firefox = discover(&root)
824+
.into_iter()
825+
.find(|row| row.browser == "firefox")
826+
.unwrap();
827+
assert_eq!(firefox.pac_url, None);
828+
assert!(!firefox.locked);
829+
fs::remove_file(root.join("etc/firefox/policies/policies.json")).unwrap();
830+
let firefox = discover(&root)
831+
.into_iter()
832+
.find(|row| row.browser == "firefox")
833+
.unwrap();
834+
assert_eq!(firefox.pac_url.as_deref(), Some(PAC));
835+
fs::remove_dir_all(root).unwrap();
836+
}
837+
656838
#[test]
657839
fn legacy_keys_split_across_files_are_merged() {
658840
let root = temp_root("split");

0 commit comments

Comments
 (0)