Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs | Hacker News Reader

Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs

github.com

58 pointsrootVIII36 comments

https://github.com/rootVIII/pdfinverter/blob/master/utils.go...

using go to wrap a python script - this is some "hacker man" stuff right here

And written to be less readable with e.g.:

    return bytes.ReplaceAll(script, []byte{0x09}, []byte{0x20, 0x20, 0x20, 0x20})

instead of

    bytes.ReplaceAll(script, []byte("\t"), []byte("    "))

Skimming through the code, there's a lot of other pretty basic mistakes.

The most egregious is that the use of a python script has a security issue. Let's see if you can spot it, it's in these lines:

    tmp := fmt.Sprintf("/var/tmp/invertpdf--%s/", time.Now().Format("20060102150405"))
    os.Mkdir(tmp, 0700)
    WriteText(tmp+"pngtopdf.py", GetPDFConv())
    // later execute that .py file

So, what's the issue? Well, using a predictable temporary directory and then not checking for an error in `mkdir` means that an attacker can easily create that directory before you do (especially since it's a predictable name based on the time), and then write their own python script. That lets another user on your machine run arbitrary code as your user.

"But", you might say, "WriteText does an os.Exit if it can't write the file". That doesn't matter. If i create the directory with permissions 777, and then have a program waiting for the python script to be written in order to replace it with a malicious script, Mkdir will error (dir already exists), but WriteText will succeed, and so the vulnerability still happens.

This is the sort of dumb vulnerability you get if you don't know that `ioutil.TempDir` exists (or don't know about symlink races, tmpdir races, etc). `ioutil.TempDir("/var/tmp", "invertpdf-")` would be the more secure way to do this, though obviously you still should check that error too.

There's a lot of other problems with this program, but this vulnerability is the most obvious.

Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!

It took me a few to respond, but I'll make some recommendations now:

1. Use filepath.Glob if you want to glob

Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do.

2. ExitErr shouldn't be used, especially not within nested functions.

You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr. You should be instead doing `return nil, err` all the way back up to `err := RunApp()`, and printing the error there. That will let defers run. In general, using os.Exit anywhere other than in the main function is an antipattern.

3. Your parallel processing should be using a waitgroup or errgroup. A slice of channels is super fragile.

In [1], do something like the following:

    // before the for loop
    var wg sync.WaitGroup
   
    // in each iteration of the for loop
    wg.Add(1)
    go func(name string) {
      defer wg.Done()
      cli.ImageRoutine(fileName)
    }(fileName)

    // at the end, to wait
    wg.Wait()

Use an errgroup if you need to cancel it.

The channel thing you're doing now is more fragile than a waitgroup, and harder to reason about.

4. use of exported functions / unexported functions is all over the place and inconsistent.

There's a few other things, but that's what I've got offhand. I can't actually run the code because I only use linux, and it obv doesn't run on linux. Oh, I guess that's another thing, you want some build tags to make it more clear it won't run on linux.

Hopefully something in there was helpful

[0]: https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d...

[1]: https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc7254514...

Hi TheDong. I put most of the changes you recommended and I am much happier with the overall project. I decided to remove the Python part though and went back to the Imagick bindings. There's probably still some things to fix but thanks again for taking the time to look and advise.

okay have some of these fixes in progress. Thanks again for taking the time to look at my code! Very appreciative

Well any old working directory that exists at startup is removed. So an attacker would not be able to create the directory before the user runs the program. And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700. Also, I would rather specify the actual bytes directly and force a lazy programmer to look at the ASCII table over using []byte(" "). These are just some of my after-thoughts.

> So an attacker would not be able to create the directory before the user runs the program

I said it was a race. An attacker would race with the program's deletion of the directory to recreate it. This race is really easy to do, and there have been numerous CVEs for this sort of race.

> And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700

That's not true.

If I own a directory, I can delete and recreate the files in that directory, even if I don't own the file, even if the file is 0700. Feel free to experiment around to see this.

Hmm okay. I'm going to start fixing that part and also attempt some CGO/objective-C to replace the python dependency. I had figured that removing any possible directory that could be seen as an application directory (at app-startup) would alleviate the issue you described. Thanks for the feedback though and also taking the time to look.

But using boring Python only, you are unable to add fancy Go mention to the submission's title!

Using Go to wrap a Python script that is mostly just a wrapper for some macOS APIs.

It may be possible to replace the dependency on the python script by using objective c to call the required libraries, then expose a c API that can be called from the go application. E.g. https://gist.github.com/porty/6b94f7f908ed7af0f070 https://github.com/caseymrm/menuet

That said, I've seen messier things than python-in-go shipped and used reliably in production to solve business problems.

That's a fantastic idea. I actually looked into it because I really didn't want to use Python to create the output PDFs. I have experience making C bindings in Go, but had issues with Objective C (I also don't know Obj-C). If you are able to remove the Python dependency with a CGo please open a PR! Thanks for the feedback too...

That or just write the whole script in Python and avoid a needless Go dependency.

Seems to invert RGB values without performing a hue rotation, that leads to colors also being changed, not just the brightness.

A browser plugin that I use for Safari/Chrome/Firefox/Edge does this too (https://darkreader.org).

> Configure the dark theme: brightness, contrast and sepia. Enable for all websites or particular domains.

Works for PDFs too.

No affiliation.

I like dark reader too, with the most gpu intensive mode and sepia it makes for some really good dark modes for most websites. And I’ve built dark mode css off of it as an inspiration.

Edit: the "Dynamic" (GPU) mode doesn't work on all sites. On HN I'll use Filter+ with the settings -20 brightness, contrast off, sepia +30, grayscale off.

While on Github I'll do -5, 0, +30, 0. Mainly just tweaking the brightness while having Sepia on most sites.

Not sure why this needs a dedicated application, you can do this in Chrome or FF with a single line of JS:

  javascript:(function(){viewer.style = 'filter: grayscale(1) invert(1) sepia(1) contrast(75%)';})()

Hmm, for me, following works with Google Chrome PDF viewer:

Bookmarklet

javascript:(function() { var v = document.getElementsByTagName("html"); v[0].style.background = "white"; v[0].style.filter = "invert(90%) sepia(60%) brightness(70%)"; v[0].style.backgroundColor = "black"; document.getElementsByTagName("body")[0].style.background = "white"; })();

That does not work with the Chrome PDF viewer for me. (Chrome 87, Mac)

Which PDF viewer? 87 has a test running for a new version.

chrome://flags/#pdf-viewer-update

Didn't work for me. I'm using Brave.

You can do this on-the-fly with Zathura... in Linux

Zathura also exists on macOS..

You can also do it in Okular.

Also GNOME's default PDF reader. Don't know what it's called though.

Edit: It appears to be Evince [0]

[0] https://en.wikipedia.org/wiki/Evince

yep, seems to be built right into gnome on Ubuntu https://imgur.com/a/CA6zoWf

mupdf also supports this.

Very cool. I've been searching for a PDF viewer which can do this under macos, but just converting is fine too.

So PDF viewers apps on MacOS do not support color changing by way of accessibility options?

Why would that be a feature of an application? That’s an OS function (https://www.apple.com/accessibility/mac/vision/)

one bad thing about this is that if you also like to redshift your color temperature, you cannot have that and the OS-level invert on at the same time (last time I checked). Hence, inverting the pdf file itself allows you to have the best of both worlds.

In the Sumatra pdf reader you can edit the config so it inverts the colors.

I like it, thanks!

We went full dark mode when we released the 2.0 version of our app which has PDF and EPUB support:

https://getpolarized.io/2020/10/05/Polar-2-0-Release.html

the issue we had was that we were using a CSS filter like other commenters mentioned.

The problem is that it inverts images too and pdfjs doesn't actually specify which parts of the document are an image as it's just writing to a canvas.

My plan moving forward is to fix pdfjs so it can invert natively and the actual canvas is inverted so that images won't be inverted.

I think figures would STILL be a problem though.

EPUBs are much easier and we're not inverting there because we can see which one is an img.

Didn’t the getpolarized spam used to come from a different account?

Reply on news.ycombinator.com