[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

Fix attachment.js error handling with complex archive #3035

Merged
merged 14 commits into from
Apr 6, 2022
Merged

Fix attachment.js error handling with complex archive #3035

merged 14 commits into from
Apr 6, 2022

Conversation

analogic
Copy link
Collaborator
@analogic analogic commented Apr 4, 2022

Fixes # (didn't know that external attachment plugin differs with native attachment.js)

Changes proposed in this pull request:

  • whole unarchiveing process rewritten into multiple functions to be more intuitive
  • fixed error handling flow
  • more of async/await
  • latest debian's bsdtar produce different message for encrypted archives - don't know if other platforms has same bsdtar or not so I've added some tests
  • not sure if tests will work since it depends on bsdtar, please guide me if something there needs to be done

Checklist:

  • docs updated
  • tests updated
  • Changes updated

@msimerson
Copy link
Member

running npm run lintfix will address all of the lint issues. I'm still looking at the PR, but I'm liking the es6 updates.

@msimerson
Copy link
Member

Looks like you can install bsdtar by adding this to the .github/workflows/ci:

➜  haraka git:(master) ✗ git diff
diff --git a/.github/workflows/ci-test.yml b/.github/workflows/ci-test.yml
index 6eaff2ef..ef4d0d29 100644
--- a/.github/workflows/ci-test.yml
+++ b/.github/workflows/ci-test.yml
@@ -36,6 +36,9 @@ jobs:
       with:
         node-version: ${{ matrix.node-version }}
 
+    - name: Install bsdtar
+      run: sudo apt-get install -y bsdtar
+
     - name: Install
       run: npm install
 

@analogic
Copy link
Collaborator Author
analogic commented Apr 4, 2022

The only thing I've found is that it might be already included https://docs.microsoft.com/en-us/virtualization/community/team-blog/2017/20171219-tar-and-curl-come-to-windows

find_bsdtar_path() will need to be updated to find the path inside windows. Sadly I've no access to windows for couple years to check this

@msimerson
Copy link
Member

Well, I'm okay with you just putting in a prehook in the attachment plugin tests, and if you can't find bsdtar, then skip the tests.

@superman20
Copy link
Collaborator

I use Windows and I can tell you the following:

  • bsdtar is included by default only in Windows Server 2019 or higher and Windows 10 v1809 (October 2018 update) or higher
  • bsdtar is called just "tar" on Windows
  • It is located in the primary System32 folder (i.e., C:\Windows\System32). This location would typically be in the PATH by default.
  • It is known to work on older versions of Windows by copying the exe's, but there is no official way to get it on older versions, to my knowledge.

Copy link
Member
@msimerson msimerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member
@msimerson msimerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, the tests aren't running at all, because plugin.bsdtar_path isn't exported. I suggest changing line 75 from this:

            bsdtar_path = `${dir}/bsdtar`;

to this:

            plugin.bsdtar_path = bsdtar_path = `${dir}/bsdtar`;

@analogic
Copy link
Collaborator Author
analogic commented Apr 4, 2022

I've tried two blind tests but execution with absolute and relative path was failing, see:
https://github.com/haraka/Haraka/runs/5814356719?check_suite_focus=true#step:5:700
https://github.com/haraka/Haraka/runs/5814251741?check_suite_focus=true#step:5:690

(I'll do requested change tomorrow)

@msimerson
Copy link
Member
msimerson commented Apr 5, 2022

windows tests conversation moved here and deleted from this PR

@haraka haraka deleted a comment from superman20 Apr 5, 2022
@haraka haraka deleted a comment from superman20 Apr 5, 2022
@msimerson msimerson merged commit 1a02e03 into haraka:master Apr 6, 2022
@analogic
Copy link
Collaborator Author
analogic commented Apr 7, 2022

Sorry, been out of office. Thanks Matt!

@msimerson msimerson mentioned this pull request Jun 4, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recursive issue which leads to crash
3 participants