Disallow absolute file paths in specs in BUILD files

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

caveness
pants
3614, 3855
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

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
JS
  1. Thanks for catching these bugs! LGTM, but one opiniony item marked as an issue for you to accept or reject depending on your judgement.
  2. src/python/pants/build_graph/address.py (Diff revision 1)
     
     
    s|ref,'//'|ref, '//'|
  3. src/python/pants/util/strutil.py (Diff revision 1)
     
     

    It looks like the 2 usages intend "strip this prefix (once) if present"; ie: both ////address:spec and file:file:/path are unexpected and invalid. Basically the simplest meaning of strip_prefix. If you agree I think it would make sense to kill the num_instances_to_strip parameter and simply strip the prefix once if present. If someone needs to loop they can, and if two folks need to do it this more complex logic could be resurrected. My bets are on that not coming to pass though.

    1. Thanks for the feedback -- I agree that the simpler approach is better.

  4. 
      
CA
JS
  1. I'll patch this in by end of day if there is no feedback from others.
  2. 
      
NH
  1. Looks good to me! Could you update the title to reflect how the check changes behavior, maybe something like "Disallow absolute file paths in specs in BUILD files"

  2. I think these should be in their own test case as they aren't testing normalization.

    1. I've now split these out into their own test case.

  3. 
      
YU
  1. Ship It!
  2. 
      
CA
NH
  1. Ship It!
  2. 
      
CA
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/
JS
  1. Thanks Emily! This is now patched into master @ 96e0187c.

  2. 
      
Loading...