Skip to content

Move success message to app's listen callback.#225

Merged
geerlingguy merged 1 commit intogeerlingguy:masterfrom
mkrawczuk:node_example_fix
Apr 15, 2022
Merged

Move success message to app's listen callback.#225
geerlingguy merged 1 commit intogeerlingguy:masterfrom
mkrawczuk:node_example_fix

Conversation

@mkrawczuk
Copy link
Contributor

express's server.listen has an asynchronous nature and it's API enables
providing a callback that is fired upon success. Now the success messaage
is logged only when the server has been set up and listening on provided
socket address, otherwise the error stack trace is printed.

express's `server.listen` has an asynchronous nature and it's API enables
providing a callback that is fired upon success. Now the success messaage
is logged only when the server has been set up and listening on provided
socket address, otherwise the error stack trace is printed.
@mkrawczuk
Copy link
Contributor Author

Explaination:
Previously, if anything would go wrong, first a success message would be printed, then the error stack trace. Might bring a lot of confusion to someone unfamiliar.

Additional comment:
Binding to port 80 (and all the ports no. < 1024) is only allowed for root, and running a node application as root is a huge security problem. A solution for this would be something along un-becoming super-user for the forever start... task, changing the port no. to 8088, and forwarding traffic from 80 to 8088 with an Ansible task using the iptables module:

    - name: Configure iptables to forward port 80 to port 8088 on all interfaces.
      iptables:
        table: nat
        chain: PREROUTING
        protocol: tcp
        match: tcp
        destination_port: "80"
        jump: REDIRECT
        to_ports: "8088"

Such solution would enforce good practice, but since this not a web security textbook it's probably best be left as-is for the sake of simplicity.

@geerlingguy geerlingguy merged commit a7b0308 into geerlingguy:master Apr 15, 2022
@geerlingguy
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants