Skip to content
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

Remove overly restrictive advice on stable, opaque ids #364

Closed
lcartey opened this issue Apr 10, 2019 · 1 comment
Closed

Remove overly restrictive advice on stable, opaque ids #364

lcartey opened this issue Apr 10, 2019 · 1 comment
Labels

Comments

@lcartey
Copy link

lcartey commented Apr 10, 2019

The specification currently has this to say about rule identifiers:

In the case of a rule, id SHALL contain a containing a stable, opaque identifier for the rule.

Note the SHALL. This means that no producers should ever populate the id field with something that is not opaque.

In justification, the specification says:

Rule identifiers should be opaque – that is, they should not convey information to a user – because a rule's implementation might change over time. Suppose a rule id is "DoNotDoXOrY", suppose circumstances change so that “Y” is now acceptable, and suppose the implementation of the rule changes accordingly. Because the rule id must not change, the string "DoNotDoXOrY" will continue to be persisted to logs, where it will convey outdated guidance to users in a way that an opaque identifier such as "CA2101" would not.

I think this is overly restrictive because:

  1. Many tools do produce non-opaque unique identifiers in practice. For example, we at Semmle use readable ids (such as cpp/path-injection), as do findbugs (http://findbugs.sourceforge.net/bugDescriptions.html#BC_EQUALS_METHOD_SHOULD_WORK_FOR_ALL_OBJECTS) and tslint (https://palantir.github.io/tslint/rules/). These tools simply do not have an opaque id for their rules, which means producers of SARIF for these tools should not currently populate the rule id fields.
  2. However, the spec also says "A reportingDescriptor object SHALL contain a property named id whose value is a string.", which means a producer has to populate the id field!
  3. In any case. the justification is weak, because we now have a mechanism in the format (deprecatedIds) for tracking results across the renaming of rule ids. In the example given, the rule id could be renamed from "DoNotDoXOrY" to "DoNotDoX". We actually have a similar mechanism for renaming our ids at Semmle.

My suggestion would be to remove the opaque requirement, or at least downgrade it to a "recommendation".

@ghost
Copy link

ghost commented Apr 14, 2019

@lukecartey To accommodate tools that do not emit opaque rule ids, I rephrased §3.25.5 (result.ruleId) and §3.46.3 (reportingDescriptor.id) to say that rule ids SHALL be stable but only SHOULD be opaque. I did not rephrase or remove any of the text praising the virtues of opaque ids. There are actually good reasons to use them, which the text does not do a good job of explaining, but I can take care of that editorially during the comment period (or if I have time before Wednesday).

For now I'll just mention that deprecatedIds isn't intended to give tool developers license to change their rule ids. It's intended for the scenario where the tool developers decide they've made a mistake, for example, that a rule is too broad and needs to be split into two rules. The example in §3.46.4 (reportingDescriptor.deprecatedIds) illustrates that scenario.

@michaelcfanning FYI

@ghost ghost self-assigned this Apr 14, 2019
@ghost ghost added 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. impact-non-breaking-change labels Apr 14, 2019
@ghost ghost added the resolved-fixed label Apr 14, 2019
@ghost ghost closed this as completed Apr 14, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant