Bug 100805 – garden-o-matic should work for local results

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

garden-o-matic should work for local results

[Dirk Pranke]

Created attachment 171569 [details]
Patch

[Dirk Pranke]

Next step in getting g-o-m to replace rebaseline-server. This roughly works for examining the failures for a single bot, but the UI is clearly the g-o-m UI and is not yet tuned for this use case.

I haven't yet tried the "rebaseline" part yet (but I suspect it'll just work).

Patch still needs tests and another once-over for cleanup. 

Adam, any thoughts on the approach to serving the local files statically? It seems like I'm fighting w/ CSP and CORS here and perhaps there's a better way?

[Ojan Vafai]

Comment on attachment 171569 [details]
Patch

LGTM. I'll leave it for Adam to take a look though.

[Adam Barth]

Comment on attachment 171569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171569&action=review

This seems like a reasonable approach.  I'm surprised we didn't need to add CORS headers.  Perhaps we do that already?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/garden-o-matic.html:35
> -                                         img-src 'self' https://ajax.googleapis.com http://build.chromium.org http://build.webkit.org file:;
> +                                         img-src 'self' https://ajax.googleapis.com http://build.chromium.org http://build.webkit.org file: http://127.0.0.1:8127;

You'll probably need to add this to media-src and frame-src as well.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:192
> +    return resultsDirectoryListingURL(platform, builderName) + 'results/layout-test-results';

Why did you delete the kLayoutTestResultsPath constant?

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:139
> +        if not filesystem.isabs(path) and self.server.options.results_directory:
> +            fullpath = filesystem.abspath(filesystem.join(self.server.options.results_directory, path))

Can we add some to defend against directory traversal here?  Perhaps check that fullpath is actually inside results_directory?

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 171569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171569&action=review
> 
> This seems like a reasonable approach.  I'm surprised we didn't need to add CORS headers.  Perhaps we do that already?
> 

Nope, they're turned off. Which requests did you think needed CORS?

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/garden-o-matic.html:35
> > -                                         img-src 'self' https://ajax.googleapis.com http://build.chromium.org http://build.webkit.org file:;
> > +                                         img-src 'self' https://ajax.googleapis.com http://build.chromium.org http://build.webkit.org file: http://127.0.0.1:8127;
> 
> You'll probably need to add this to media-src and frame-src as well.
> 

Will do.

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:192
> > +    return resultsDirectoryListingURL(platform, builderName) + 'results/layout-test-results';
> 
> Why did you delete the kLayoutTestResultsPath constant?
>

It was only being used in one place and I had to change it anyway to get rid of the duplication between
resultsDirectoryURL() and resultsDirectoryListingURL().
  
> > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:139
> > +        if not filesystem.isabs(path) and self.server.options.results_directory:
> > +            fullpath = filesystem.abspath(filesystem.join(self.server.options.results_directory, path))
> 
> Can we add some to defend against directory traversal here?  Perhaps check that fullpath is actually inside results_directory?

Good idea. Will do.

[Dirk Pranke]

Created attachment 171704 [details]
update w/ review feedback, clean up, add tests

[Dirk Pranke]

Committed r133061: <http://trac.webkit.org/changeset/133061>