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

ENH: more obscurity - add ', ", and {} to obscure filenames #4957

Merged
merged 6 commits into from Oct 7, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Sep 24, 2020

Also refactors a bit how obscure file name is generated so trailing white spaces are PARTS now thus code is simplified.
Also removed ad-hoc windows skipping of a name with trailing space -- testing now mkdir as well. Let's see if that would allow those previously failing tests to pass and what would be the name on windows.

The most obscure filename one my laptop now is "\';a&b&cΔЙקم๗あ `|.

I have done only a limited sweep over local tests.

note FTR: in conda python 3.5 I have two ria tests failing but it seems unrelated to this (datalad/core/distributed/tests/test_push.py:test_ria_push datalad/core/distributed/tests/test_clone.py:test_ria_postclonecfg)

@yarikoptic
Copy link
Member Author

so it does flip (only) two tests - something to address
======================================================================
ERROR: datalad.interface.tests.test_run_procedure.test_quoting
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/tests/utils.py", line 554, in _wrap_with_tree
    return t(*(arg + (d,)), **kw)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/tests/test_run_procedure.py", line 298, in test_quoting
    ds.run_procedure(spec=["just2args", "with ' sing", 'with " doub'])
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/distribution/dataset.py", line 503, in apply_func
    return f(**kwargs)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 494, in eval_func
    return return_func(generator_func)(*args, **kwargs)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 482, in return_func
    results = list(results)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 413, in generator_func
    allkwargs):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 552, in _process_results
    for res in results:
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/run_procedure.py", line 452, in __call__
    return_type='generator'
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 413, in generator_func
    allkwargs):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 552, in _process_results
    for res in results:
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/core/local/run.py", line 239, in __call__
    sidecar=sidecar):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/core/local/run.py", line 615, in run_command
    outputs=outputs)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/core/local/run.py", line 450, in format_command
    return sfmt.format(command, **kwds)
  File "/opt/python/3.7.1/lib/python3.7/string.py", line 186, in format
    return self.vformat(format_string, args, kwargs)
  File "/opt/python/3.7.1/lib/python3.7/string.py", line 190, in vformat
    result, _ = self._vformat(format_string, args, kwargs, used_args, 2)
  File "/opt/python/3.7.1/lib/python3.7/string.py", line 230, in _vformat
    obj, arg_used = self.get_field(field_name, args, kwargs)
  File "/opt/python/3.7.1/lib/python3.7/string.py", line 295, in get_field
    obj = self.get_value(first, args, kwargs)
  File "/opt/python/3.7.1/lib/python3.7/string.py", line 250, in get_value
    return args[key]
IndexError: list index out of range
======================================================================
ERROR: datalad.plugin.tests.test_plugins.test_wtf
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/tests/utils.py", line 554, in _wrap_with_tree
    return t(*(arg + (d,)), **kw)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/plugin/tests/test_plugins.py", line 97, in test_wtf
    wtf(dataset=ds.path, sensitive=sensitive)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 494, in eval_func
    return return_func(generator_func)(*args, **kwargs)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 482, in return_func
    results = list(results)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 413, in generator_func
    allkwargs):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 552, in _process_results
    for res in results:
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/plugin/wtf.py", line 398, in __call__
    infos[s] = section_callables[s]()
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/plugin/wtf.py", line 244, in _describe_dataset
    result_renderer='disabled', on_failure='ignore')
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 494, in eval_func
    return return_func(generator_func)(*args, **kwargs)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 482, in return_func
    results = list(results)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 413, in generator_func
    allkwargs):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/interface/utils.py", line 552, in _process_results
    for res in results:
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/metadata/metadata.py", line 1061, in __call__
    **res_kwargs):
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/metadata/metadata.py", line 206, in query_aggregated_metadata
    agginfos, agg_base_path = load_ds_aggregate_db(ds)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/metadata/metadata.py", line 845, in load_ds_aggregate_db
    info_fpath, agg_base_path = get_ds_aggregate_db_locations(ds, version, warn_absent)
  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/metadata/metadata.py", line 800, in get_ds_aggregate_db_locations
    info_glob = op.join(ds.path, agginfo_relpath_template).format('*')
IndexError: tuple index out of range

… run_procedure

Run is used by `run_procedure` to centralize execution logic. But
`Run` also passes command through .format.  AFAIK neither dataset path
nor procedure script path are expected to be able to take advantage
from .format() functionality, but if either of their paths contains {}
or some {var} it could either cause an error or unexpected formatting.

guard_for_format used for ds path and procedure path, would replace {
and } with {{ and }}, thus making it safe for created command to be
passed to .format.
@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Oct 5, 2020
@yarikoptic
Copy link
Member Author

I have fixed for run_procedure. @kyleam @mih @bpoldrack please review, in particular 6f92f3c, since I think that could be relevant to all things of run and HIRNI.

Cheers,

@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

Merging #4957 into maint will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4957      +/-   ##
==========================================
+ Coverage   89.68%   89.81%   +0.13%     
==========================================
  Files         289      292       +3     
  Lines       40544    40726     +182     
==========================================
+ Hits        36362    36580     +218     
+ Misses       4182     4146      -36     
Impacted Files Coverage Δ
datalad/interface/run_procedure.py 90.79% <ø> (ø)
datalad/utils.py 82.37% <ø> (+0.31%) ⬆️
datalad/metadata/metadata.py 85.79% <100.00%> (ø)
datalad/tests/utils.py 86.81% <100.00%> (+0.02%) ⬆️
datalad/downloaders/tests/test_s3.py 51.94% <0.00%> (-20.15%) ⬇️
datalad/downloaders/s3.py 40.51% <0.00%> (-17.16%) ⬇️
datalad/downloaders/http.py 81.85% <0.00%> (-2.71%) ⬇️
datalad/downloaders/tests/test_http.py 88.22% <0.00%> (-1.71%) ⬇️
datalad/core/distributed/clone.py 88.75% <0.00%> (ø)
datalad/downloaders/tests/test_credentials.py 100.00% <0.00%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37fa322...5db2c54. Read the comment docs.

@mih
Copy link
Member

mih commented Oct 5, 2020

What would happen to any caller that already passes {{..}}, wouldn't this come out as {{{{...}}}} now?

FTR: sidenote (not confirmed): I see special handling of windows being done. It would be good to evaluate whether that is in effect much. I think tests accumulated special cases for this platform to define system-appropriate levels of obscurity in some (many?) places.

#2929

@kyleam
Copy link
Contributor

kyleam commented Oct 5, 2020

What would happen to any caller that already passes {{..}}, wouldn't this come out as {{{{...}}}} now?

I'm not spotting a case where that would happen. 6f92f3c only escapes brackets in the dataset path and procedure file argument that run_procedure uses to construct the command it passes to run.

@yarikoptic
Copy link
Member Author

FTR: sidenote (not confirmed): I see special handling of windows being done

just to make sure: I don't think this PR introduces any new special handling for windows.

@yarikoptic
Copy link
Member Author

yarikoptic commented Oct 5, 2020

... and procedure file argument

that was my question somewhat -- does any functionality (seems not, according to the tests) expects procedure name/file itself being subject to ".format()" based on some parameters in the config etc?

edit 1: probably not, and if it does -- such .format() ing anyways would need to happen earlier I guess to deduce how to run the procedure script etc. So I guess it should be of no concern at the point of where I disabled formating procedure script filename.

@kyleam
Copy link
Contributor

kyleam commented Oct 5, 2020

that was my question somewhat -- does any functionality (seems not, according to the tests) expects procedure name/file itself being subject to ".format()" based on some parameters in the config etc?

Not that I'm aware of. Such a feature would be pretty surprising to me conceptually, and, as you already noted in the edit, the run_procedure code gives no hint of providing such handling.

@yarikoptic
Copy link
Member Author

ok then, let's proceed

@yarikoptic yarikoptic merged commit d071bcd into datalad:maint Oct 7, 2020
@yarikoptic yarikoptic deleted the bf-obscure% branch October 7, 2020 15:06
@yarikoptic yarikoptic added this to the 0.13.5 milestone Dec 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants