daveallie / entangler

Two way file syncer using platform native notify
MIT License
8 stars 6 forks source link

Prevent File.read on directories #13

Closed SamODonnell89 closed 6 months ago

SamODonnell89 commented 6 months ago

Example of the error

image

A simple way to reproduce the problem

`mkdir test_dir`
full_path="${Dir.pwd}/test_dir"

File.exist(full_path) # => true
File.read(full_path) #=> Is a directory @ io_fread - /home/ruby/core/repo/test_dir (Errno::EISDIR)
daveallie commented 6 months ago

Out of curiosity, is the /rails/test/integration/payroll/us/import_export path a symlink to a directory? As far as I can tell, that would be the only reason this would happen, the Listen gem should only process file changes, not directory changes, however it does process symlink changes. EntangledFiles are only created from Listen's listener.

Otherwise, changes look good

SamODonnell89 commented 6 months ago

Out of curiosity, is the /rails/test/integration/payroll/us/import_export path a symlink to a directory? As far as I can tell, that would be the only reason this would happen, the Listen gem should only process file changes, not directory changes, however it does process symlink changes. EntangledFiles are only created from Listen's listener.

Otherwise, changes look good

I'm not clear on that at the moment, one of our devs kept running into the issue. I don't see how they would accidentally create a sym link directory. We do have code that was added to prevent permission issues surrounding docker files. E.G., bin/rails g model foo would create a file with permissions that don't match and cause entangler to crash.

Another member of the PIT team fixed that issue by adding

# Set the UID and GID to custom values; default to 1000:1000 which matches
# the file permissions of our ubuntu:ubuntu user on the devbox instance.
ARG UID=1000
ARG GID=1000

# Run and own only the runtime files as a non-root user for security. NOTE: disable this for debugging
RUN groupadd -g "${GID}" rails
RUN useradd rails -u "${UID}" -g "${GID}" --create-home --shell /bin/bash && \
    mkdir -p db log tmp /rails /tmp/rails && \
    chown -R rails:rails db log tmp /rails /tmp/rails

I don't think that would cause symlink issues but I have no better idea atm.

daveallie commented 6 months ago

I suspect it'd be more related to your test setup, as an example, in a before test, you created region-specific folders and then symlinked a nested import_export folder back to a central folder. Regardless, the change looks good, will get a patch version out.

daveallie commented 6 months ago

Should be live in 1.3.2