search

cea-hpc / modules

Environment Modules: provides dynamic modification of a user's environment
http://modules.sourceforge.net/
GNU General Public License v2.0
695 stars 109 forks source link

Protect environment variables #445

Closed adrien-cotte closed 2 years ago

adrien-cotte commented 2 years ago

Hi,

Here is my PR for issue https://github.com/cea-hpc/modules/issues/429. Let me know if something is wrong.

Best, Adrien

codecov[bot] commented 2 years ago

Codecov Report

Merging #445 (fc4b411) into master (b8bd3c4) will decrease coverage by 0.04%. The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
- Coverage   99.29%   99.25%   -0.05%     
==========================================
  Files          12       12              
  Lines        4539     4544       +5     
==========================================
+ Hits         4507     4510       +3     
- Misses         32       34       +2
Impacted Files Coverage Δ
tcl/init.tcl.in 96.25% <ø> (ø)
tcl/envmngt.tcl.in 99.25% <81.25%> (-0.50%) :arrow_down:

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 b8bd3c4...fc4b411. Read the comment docs.

xdelaruelle commented 2 years ago

This is progressing well. I have changed a little bit the doc and the warning message.

It is now time to craft specific tests to validate the correct behavior of the configuration when not set, set with valid or invalid values. Tests should cover attempts to modify variables with append-path, prepend-path, remove-path, setenv, unsetenv and source-sh.

Tests should cover when these changes occur on the different modulefile evaluation mode (load, unload, help, display, test, refresh, whatis). Test should also cover when protected variables are modified by append-path, prepend-path and remove-path sub-commands.

Also there should be a test when Modules-specific variable (like LOADEDMODULES) is defined in protected_envvars.

adrien-cotte commented 2 years ago

Hey @xdelaruelle, many thanks for your assistance!

It is now time to craft specific tests to validate the correct behavior of the configuration when not set, set with valid or invalid values. Tests should cover attempts to modify variables with append-path, prepend-path, remove-path, setenv, unsetenv and source-sh.

Tests should cover when these changes occur on the different modulefile evaluation mode (load, unload, help, display, test, refresh, whatis). Test should also cover when protected variables are modified by append-path, prepend-path and remove-path sub-commands.

Agreed, but could you help me (again) to find the right files to modify or create, please?

Also there should be a test when Modules-specific variable (like LOADEDMODULES) is defined in protected_envvars.

Good point, which behavior do you want for those use cases? Maybe we should return an error if protected_envvars contains a "forbidden var" (LOADEDMODULES, MODULEPATH, MODULESHOME, MODULES_CMD, _LMFILES_ etc...). Is there a easy way to get an exhaustive list?

xdelaruelle commented 2 years ago

Good point, which behavior do you want for those use cases? Maybe we should return an error if protected_envvars contains a "forbidden var" (LOADEDMODULES, MODULEPATH, MODULESHOME, MODULES_CMD, _LMFILES_ etc...). Is there a easy way to get an exhaustive list?

For the initial implementation maybe it could be left as is, so if a user adds a Modules-specific variable in the protected list, module will not work correctly anymore. Will see if such strategy will fit on the long term. We could look later on at implementing a special treatment for these variables.

xdelaruelle commented 2 years ago

It is now time to craft specific tests to validate the correct behavior of the configuration when not set, set with valid or invalid values. Tests should cover attempts to modify variables with append-path, prepend-path, remove-path, setenv, unsetenv and source-sh. Tests should cover when these changes occur on the different modulefile evaluation mode (load, unload, help, display, test, refresh, whatis). Test should also cover when protected variables are modified by append-path, prepend-path and remove-path sub-commands.

Agreed, but could you help me (again) to find the right files to modify or create, please?

I suggest you create a new test file in modules.70-maint, named 410-protected_envvars.exp. You could base its content on modules.50-cmds/540-complete.exp.

Along this test file, I suggest you add a protect/1.0 modulefile in modulefiles.3 modulepath. Content of this modulefile could be based on complete/1.0. Having a TESTSUITE_PROTECT selection variable instead of TESTSUITE_COMPLETE. Each case of the switch in this modulefile may corresponds to a modulefile command to test (append-path, setenv, unsetenv, etc).

xdelaruelle commented 2 years ago

@adrien-cotte I have just updated your branch to update the documentation commit and make it comply with the content that is now expected for configuration option and environment variable description.

xdelaruelle commented 2 years ago

@adrien-cotte Sorry, I have made a mistake when trying to re-update your branch against the master branch update I have pushed this morning. I have mistakenly pushed the current master branch against your master branch, which was also used for this PR. It has closed the PR. I have re-uploaded your branch on my test repository, so you can fetch there to include it in your github repo and recreate a PR from that.