README examples are not valid for 3.x

[kingthorin]

https://github.com/shapesecurity/salvation#query-a-policy doesn't work. URI.parse doesn't exist, though URI.parseURI does (should probably be parseUri for proper java camelCase 😉 ). But, even with parseURI:

The method allowsExternalScript(Optional<String>, Optional<String>, Optional<URLWithScheme>, Optional<Boolean>, 
Optional<URLWithScheme>) in the type Policy is not applicable for the arguments (Optional<String>, 
Optional<String>, Optional<Optional<URI>>, Optional<Boolean>, Optional<URLWithScheme>)

Also I'm wondering what the new functionality for merging (intersecting) policies is? (In the case of multiple CSP headers, or header + meta tag.)

For context: Salvation is used by Zaproxy to analyze CSP for users. ex: https://github.com/zaproxy/zap-extensions/blob/master/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/ContentSecurityPolicyScanRule.java I'm looking at updating that passive scan rule to use Salvation2 (3.x).

Edit: Also, although 3.0.0 was tagged in the repo and released to Maven it wasn't made into a release in the repo:

[bakkot]

Ah, sorry, I tweaked the API just before landing and didn't update the readme. I'll fix that today.

what the new functionality for merging (intersecting) policies is?

There's no API for it because it's not possible to do right: in many cases it is simply unrepresentable. For example, no single policy can express the intersection of the behavior of the two policies script-src 'unsafe-inline' plus script-src 'nonce-asdf' (which together will allow only those scripts which are both inline and have a nonce). The previous implementation was broken.

(It also depends a lot on what browsers someone cares to support - e.g. if someone supports Safari you can't assume that 'strict-dynamic' works.)

At a glance, I think you want to just parse each policy separately, and then query each thing in the list in turn. It looks like you're going for "do these collectively allow this unsafe behavior?", so you'd want to do something like boolean allowsUnsafeInlineScripts = policyList.stream().allMatch(p -> p.allowsInlineScript(Optional.empty(), Optional.empty(), Optional.empty());. If you'd like I can add most of the high-level querying APIs to PolicyList; they'd do basically that.

it wasn't made into a release in the repo

Done.

[kingthorin]

Thanks @bakkot!

[kingthorin]

I need a way to check if directives allow wildcard sources. With 2.7.x we were doing something like:

    private static final String WILDCARD_URI = "http://*";
    private static final Optional<URI> PARSED_WILDCARD_URI = URI.parseURI(WILDCARD_URI);
...
    if (pol.allowsScriptFromSource(PARSED_WILDCARD_URI)) {
        // Tell the user this is a bad plan"
    }

How would I do something similar with the new allowsExternalScript? I understand that defining a wildcard URI might not be great but we could use something like "https://ShouldNeverAppearInALiveCSP.net" (as a fictional source to see if it's permitted).

[bakkot]

What counts as a wildcard source? script-src http: https: ftp: will allow exactly the same scripts as script-src * unless you are serving a webpage from "file://" or some other unusual protocol. Do you intend to distinguish those two cases?

[kingthorin]

Could check all three schemes with the fictional domain in a loop.

[bakkot]

Yeah, I'm just asking what information you are trying to query so I can better tell you how to write the query. If you just want to know "does this have script-src *", you can ask that directly: policy.getFetchDirective(FetchDirectiveKind.ScriptSrc).map(d -> d.star()).orElse(false), say. (You'd have to check default-src and script-src-elem like that as well, or use getGoverningDirectiveForEffectiveDirective(FetchDirectiveKind.ScriptSrcElem) to figure out which is applicable - though note that script-src-elem is not yet widely supported, so you may want to check both it and script-src explicitly anyway.)

But if what you care about is "is this excessively permissive about where it allows scripts from", you'd have to write something else, and then you have to know what you mean by "excessively permissive" before you can decide how to write that.

[kingthorin]

Oh okay sorry I misunderstood.

But if what you care about is "is this excessively permissive about where it allows scripts from", you'd have to write something else, and then you have to know what you mean by "excessively permissive" before you can decide how to write that.

Is basically what I'm after. Which would catch both script-src *, and your earlier example: script-src http: https: ftp:. There are still ways that things can be overly permissive per domain etc. but it isn't really reasonable to try to ferret that out (IMHO).

Mainly we want to catch things where a dev put in a * for development/testing purposes and then accidentally rolled it out. Or, a situation like 'I was told to implement CSP, so I did .... they never said it had to be a "strong" CSP".

I'm just using script-src as an example. It'll be done for a bunch of directives but I'm assuming if I can get advice on one then most of them will be similar.

[bakkot]

Ah, then yeah, you want something like policy.allowsExternalScript(Optional.empty(), Optional.empty(), Optional.of(URI.parseURI("http://some-fake-domain").get()), Optional.empty(), Optional.empty()). Probably you should do it for both an http and an https domain (though I wouldn't bother with ftp since that's unlikely), and warn if either is allowed.

That says "does this policy allow any external script from "http://some-fake-domain", even if the script doesn't have a nonce or hash and might be parser-inserted?" It sounds like that's the question you want to ask. ("parser-inserted" is relevant because of strict-dynamic.)

The exact shape of the API differs for scripts vs styles vs images, etc, because not all of those parts are relevant for every query (e.g. you can't use a hash to allow an external style), but that'll always be the pattern.

[kingthorin]

Thanks!

[kingthorin]

@bakkot thanks for your tips and sorry I've let this issue drag on. I hope to get back to this next week.

[kingthorin]

@bakkot I finally got this tackled and implemented.

It would still be nice if the README was updated for others, but for my two cents you can close it...

I've also just opened a minor PR as I noticed a typo while getting things implemented and tested on my side.

Thanks ❗