Honor directive parameters after a quoted filename reference

[FireBurn]

For example, the following ACL did not match example.com:

acl foo dstdomain "file-of-domains.txt" example.com

strtokFile() implements the "read directive parameters from a quoted
file" syntax used when configuration_includes_quoted_values is off (the
documented default). Once the named file's lines were exhausted,
strtokFile() returned nullptr, signalling end-of-directive to callers
such as ACLDomainData::parse(). Any tokens following the quoted filename
on the same configuration line (e.g., "example.com" above) were silently
discarded. A parameter file reference and trailing literal parameters
are valid syntax individually; combining them on one line has always
been allowed by the grammar, so the omission was "invisible" to admins.

This bug dates back to 2008 commit f32cd13 ("Replace cnfig parser gotos
with do-while loop."), which replaced a goto that resumed parsing after
the file's last line with an unconditional "return nullptr". Every Squid
release starting with v3.1.0.1 is affected.

[rousskov]

Thank you for fixing this dangerous bug! Can you tell when this bug was introduced?

Do not worry about PR description formatting if you do not want to fix it -- I will help with that.

Please add yourself to CONTRIBUTORS.

[FireBurn]

Thanks for the review! Pushed an update addressing all of it:

• Reverted the ConfigParser.cc comment back to the one-liner and added
  the suggested inline note instead.
• Replaced the C++ unit test with a squidconf test: extended
  bad-acl-dstdomain-dupe.conf with a case that combines a quoted
  file() reference with a trailing literal domain on the same line,
  plus the referenced values file. Without the fix, the trailing
  token is silently dropped and no duplicate-domain warning fires;
  with the fix, it does. Verified against a real built squid binary
  both ways (fails without the fix, passes with it), and reran the
  rest of test-suite/squidconf/*.conf to confirm nothing else
  regressed.
• Added myself to CONTRIBUTORS.

When was this introduced: commit f32cd13 ("Replace cnfig parser
gotos with do-while loop", 2008-04-11, Amos Jeffries). Before that
commit, the file-EOF branch used "goto strtok_again;", which jumped
back to the top of the function and correctly resumed parsing via
strtok(NULL, w_space). The do-while refactor replaced that goto with
a bare "return NULL;", which exits immediately instead of falling
through to resume parsing -- an unintended side effect of a pure
control-flow cleanup. SQUID_3_0_RC1 (2007-10-01) predates this
commit; every release since the 3.1 line is affected, so this has
been around for about 18 years.

PS I've had 3 hours sleep because of the implementation and then this bug

[rousskov]

I've had 3 hours sleep because of the implementation and then this bug

I am sorry to hear that and can certainly sympathize. We are almost done here -- there is only one problem left (see the new change request)! If you prefer, I would be happy to finish this up myself -- just let me know.

I edited PR title to avoid introducing a custom low-level ConfigParser: prefix/scope.

I adjusted PR description, primarily to remove diff-makes-that-obvious parts and improve formatting. Please feel free to revert my PR description changes or adjust further.

[rousskov]

Thank you for polishing this up! I hope we are done: If automated PR and staging tests succeed, and there are no objections from others, this fix will be merged.

@FireBurn, just FYI: There is no need to squash individual commits (and force-push) because Anubis, our merge bot, squashes automatically when merging a PR. It also uses PR title/description as the merged commit message...

[rousskov]

I hope we are done: If automated PR ... tests succeed

They failed. I do not know what exactly causes those failures. The same change appears to work in my local quick-and-dirty tests, but I did not test the latest master (yet).

The missing est letters after configuration context: t in the logged message quoted below are particularly alarming (to me):

2026/07/09 21:44:38| ERROR: Can not open file bad-acl-dstdomain-dupe.domains for reading
    configuration context: t-suite/squidconf/bad-acl-dstdomain-dupe.conf(21) acl

There are other similarly malformed messages in that log as well.

I will find the time to investigate this further if you do not.

[rousskov]

I have (the bigger part of) the answer: There is another Squid bug here. I fixed that bug more than a year ago while working on other Squid configuration improvements, but that fix is still in the backlog. It explains why your (correct) changes work in my tests but not in the official Squid Project CI. I will post a dedicated PR to fix that other bug, so that your PR can be merged afterwords, hopefully without changes. Please stand by.

[yadij]

FYI: I believe the missing letters are probably tes. Likely Squid trying badly to remove a relative path prefix (eg "../").

[rousskov]

I will post a dedicated PR to fix that other bug

Posted at #2458 .

I have tested this PR changes on top of #2458 and saw the expected behavior, including a test failure if this PR primary fix change is reverted. I plan to clear this PR for merging after #2458 lands.

[rousskov]

FYI: I believe the missing letters are probably tes.

As I noted earlier, in the posted sample, the missing letters are est. The same CI log contains other missing letters as well:

    configuration context: iguration(8) acl

Likely Squid trying badly to remove a relative path prefix (eg "../").

This is a different bug. I fixed this one in December 2024. That fix is also in the backlog. Hopefully, we will eventually find a way to resolve the long-standing issues that have created (and keep growing!) that backlog, badly hurting the Squid Project, but doing so is outside this PR scope.

[yadij]

I read your OP. It seems very strange that you think that.

1. Debug::Extra << "configuration context: " << ConfigParser::CurrentLocation();
2. ConfigParser::CurrentLocation() { return ToSBuf(SourceLocation(cfg_directive, cfg_filename, config_lineno)); }
3. SourceLocation::print(std::ostream &os) { if (fileName) { os << SkipBuildPrefix(fileName); ...
4. SkipBuildPrefix(const char* path) { ... return path + ToSkip; }

Much more likely that ToSkip is somehow N+3 when it should be N instead of that logic removing est from the middle of "test-suite...".

The fix for above bug still being in the backlog would explain why the current code shows it still present.