Disallow absolute file paths in specs in BUILD files

Review Request #4221 - Created Sept. 11, 2016 and submitted

Information
Emily Caveness
pants
3614, 3855
Reviewers
pants-reviews
jsirois, patricklaw, yujiec

parse_spec() had been stripping all forward slashes from the beginning of the spec before checking the spec and was not catching invalid specs in the form /src/lang/some/target. This was due to a call to lstrip() that was intended to strip only // but was instead stripping any forward slash from the beginning of the path. (Because lstrip() treats string parameters as a set of characters, calling testString.lstrip('//') strips any forward slashes from the beginning of testString, not just //.) To resolve this, I have taken the following steps:

  • Created a new utility function strip_prefix(), which strips the specified substring from a string, if present. This function provides a way to strip a substring from the start of a string that is similar to the way lstrip() strips individual characters from the start of a string. (However, strip_prefix() strips only one instance, not all, of the specified substring.)
  • Substituted strip_prefix() for lstrip() in parse_spec() to correct stripping of all forward slashes from the beginning of the spec where stripping of // is intended.
  • Added an error check for absolute path in parse_spec.
  • Also substituted strip_prefix() for lstrip() in fetcher to strip file: prefix from beginning of url. Although lstrip() will remove file:, it will also remove any combination of the characters within file: (e.g., i:) and so could cause unintended behavior.

I will open a separate ticket in GitHub to address the fact that addresses in the following form are currently not generating an error: src/lang/some/target:target.
That issue may require different treatment (e.g., generation of a warning instead of an error).

Green CI: https://travis-ci.org/pantsbuild/pants/builds/159922415

Issues

  • 0
  • 1
  • 0
  • 1
Description From Last Updated
John Sirois
Emily Caveness
John Sirois
Nick Howard (Twitter)
Yujie Chen
Emily Caveness
Nick Howard (Twitter)
Emily Caveness
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit 96e0187c96cc799e2135597859585a85d0a33758
Author: Emily Caveness <emilycaveness@gmail.com>
Date:   Wed Sep 14 11:42:50 2016 -0600

    Disallow absolute file paths in specs in BUILD files
    
    `parse_spec()` had been stripping all forward slashes from the beginning of the spec before checking the spec and was not catching invalid specs in the form `/src/lang/some/target`. This was due to a call to `lstrip()` that was intended to strip only `//` but was instead stripping any forward slash from the beginning of the path. (Because `lstrip()` treats string parameters as a set of characters, calling `testString.lstrip('//')` strips any forward slashes from the beginning of `testString`, not just `//`.) To resolve this, I have taken the following steps:
    
    * Created a new utility function `strip_prefix()`, which strips the specified substring from a string, if present.  This function provides a way to strip a substring from the start of a string that is similar to the way `lstrip()` strips individual characters from the start of a string. (However, `strip_prefix()` strips only one instance, not all, of the specified substring.)
    * Substituted `strip_prefix()` for `lstrip()` in `parse_spec()` to correct stripping of all forward slashes from the beginning of the spec where stripping of `//` is intended.
    * Added an error check for absolute path in `parse_spec`.
    * Also substituted `strip_prefix()` for `lstrip()` in `fetcher` to strip `file:` prefix from beginning of url. Although `lstrip()` will remove `file:`, it will also remove any combination of the characters within `file:` (e.g., `i:`) and so could cause unintended behavior.
    
    I will open a separate ticket in GitHub to address the fact that addresses in the following form are currently not generating an error: `src/lang/some/target:target`.
    That issue may require different treatment (e.g., generation of a warning instead of an error).
    
    Testing Done:
    Green CI: https://travis-ci.org/pantsbuild/pants/builds/159922415
    
    Bugs closed: 3614, 3855
    
    Reviewed at https://rbcommons.com/s/twitter/r/4221/
John Sirois

Thanks Emily! This is now patched into master @ 96e0187c.

Loading...