Add models for Revel and HTML templates #473
Conversation
|
Also retargeted this one at main. Reviewers note that the first commit is #472, only review the later 3 commits here. |
| * TODO: For review: this name conflicts heavily with `getBaseVariable` above, with different and probably confusing results. Perhaps rename above to `getRootVariable`? Suggestions welcome. | ||
| * I do like using `getBase` here because it lines up nicely with the `getBase` on selector exprs and write targets. | ||
| */ | ||
| VariableWithFields getBase() { |
There was a problem hiding this comment.
How about simply getParent?
| * corresponding to `a.b[c]`. | ||
| */ | ||
| Variable getBaseVariable() { | ||
| this = TVariableRoot(result) |
There was a problem hiding this comment.
getParent()*.(VariableRoot)?
| * I do like using `getBase` here because it lines up nicely with the `getBase` on selector exprs and write targets. | ||
| */ | ||
| VariableWithFields getBase() { | ||
| exists(VariableWithFields base, Field f | this = TVariableFieldStep(base, f) | result = base) |
There was a problem hiding this comment.
this = TVariableFieldStep(result, _)?
| MethodModels() { | ||
| // signature: func (*Template).Execute(wr io.Writer, data interface{}) error | ||
| hasQualifiedName("html/template", "Template", "Execute") and | ||
| (inp.isParameter(1) and outp.isParameter(0)) |
| or | ||
| // signature: func (*Template).ExecuteTemplate(wr io.Writer, name string, data interface{}) error | ||
| hasQualifiedName("html/template", "Template", "ExecuteTemplate") and | ||
| (inp.isParameter(2) and outp.isParameter(0)) |
| RawTemplateArgument() { | ||
| exists(TemplateRender render, RawTemplateRead read, VariableWithFields var | | ||
| render.getRenderedFile() = read.getFile() and | ||
| this = var.getBase*().getAWrite().getRhs() |
There was a problem hiding this comment.
Let's have some comments showing what sort of expressions are represented here -- looks like var is somevar.somefield.someotherfield and this is being bound to rhs in somevar = rhs or somevar.somefield = rhs or somevar.somefield.someotherfield = rhs?
| render.getRenderedFile() = read.getFile() and | ||
| this = var.getBase*().getAWrite().getRhs() | ||
| | | ||
| var.getBase*() = render.getArgumentVariable() and |
There was a problem hiding this comment.
Does this need to be the same getBase* as above? If so, bind it and give it a name
There was a problem hiding this comment.
No, this is just ensuring that the render argument is a base of the result we give, because otherwise we find all variables that have the same name.
e.g.
blah.Render(arg)
var arg
arg.field = bar // we don't want to flag this| var = read.getReadVariable(render.getArgumentVariable()) | ||
| or | ||
| // if no write or use of that variable exists, no VariableWithFields will be generated | ||
| // so we try to find a parent VariableWithFields |
There was a problem hiding this comment.
Elaborate on when this can happen considering all the getBase stuff above?
| // if no write or use of that variable exists, no VariableWithFields will be generated | ||
| // so we try to find a parent VariableWithFields | ||
| not exists(read.getReadVariable(render.getArgumentVariable())) and | ||
| exists(string fieldName | fieldName = read.getFieldName() | |
There was a problem hiding this comment.
Can this only go one level back (i.e. one getBase), or can it go further?
There was a problem hiding this comment.
It can go further. I wrote this assuming any write to a parent field should count. We could probably bound it to 1 or 2 and still have pretty similar results with possibly fewer FPs, if you want to do that.
There was a problem hiding this comment.
Let's see how the results look
| exists(string controllerRe, string handlerRe, string pathRe | | ||
| controllerRe = "\\Q" + this.getReceiver().getType().getName() + "\\E" and | ||
| handlerRe = "\\Q" + this.getEnclosingCallable().getName() + "\\E" and | ||
| pathRe = "/views/" + controllerRe + "/" + handlerRe + "(\\..*)?\\.html?" |
There was a problem hiding this comment.
Comment on what the \\..* part of the regex is doing? Perhaps give an example of the form of the view html filename?
|
Similary let's have an eval ticklist here; perhaps only here, or perhaps on both PRs if they have independent eval plans |
sauyon
force-pushed
the
sauyon/html-tracing
branch
2 times, most recently
from
b3c08d5 to
715bbab
Compare
sauyon
force-pushed
the
revel
branch
2 times, most recently
from
1e5bd7b to
24724bd
Compare
sauyon
force-pushed
the
revel
branch
3 times, most recently
from
1894f49 to
781f3ab
Compare
sauyon
force-pushed
the
revel
branch
3 times, most recently
from
69d6681 to
f2a10c2
Compare
|
Seems like it'll be more effort than this to port over SSAWithFields, so I'm going to revert that commit and store it in another branch for later. |
There was a problem hiding this comment.
Looks good! Two minor requests:
- Add in the doc comments for SsaWithField and VariableWithField when one would want to use SsaWithField.similar() and when VariableWithField is better. What distinguishes the two?
- In cases where we're highlighting an XSS sink due to an ultimate sink actually in an HTML file, can we expose that somehow? I imagine path steps into the HTML document are too ambitious, but could we mention "template argument $@ is output raw here"?
Sure.
Can a query have optional arguments? |
sauyon
force-pushed
the
revel
branch
4 times, most recently
from
898dfd1 to
305221b
Compare
sauyon
force-pushed
the
revel
branch
4 times, most recently
from
423355f to
de7ceaf
Compare
| @@ -0,0 +1,2 @@ | |||
| | Revel.go:7:2:7:2 | "mime/multipart" imported but not used | | |||
|
Whoops, that should be resolved now. |
|
If we merge this all at once we should close #472, probably. |
The revel stubs are very hand-written, so I've excluded the go generate commands.
I tried to update
depstubberto do the sensible thing, but I'm still running into the limits of the reflection approach. One of these days...