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

feat: Modified logger to log to file that supports fail2ban #284

Merged
merged 2 commits into from Aug 22, 2022

Conversation

StevenJCorreia
Copy link
Contributor

This PR aims to add the ability to expose Planka's logs to a logfile that can later be watched by fail2ban.

The logfile will mirror the console logs that already via sails.js with two additions:

  • Failed login attempt due to invalid username/email
  • Failed login attempt due to invalid password

@StevenJCorreia
Copy link
Contributor Author

Linked to #283.

@meltyshev
Copy link
Member

Thank you for your work!

Everything looks great, but when I decided to test, I get a 500 error when I enter the wrong login or password during development. I also don’t see any errors in the server console, just this one in the response:

SyntaxError: Unexpected token 'T', "TypeError:"... is not valid JSON
    at parse (<anonymous>)

I'm trying to figure out what's causing it.

@StevenJCorreia
Copy link
Contributor Author

@meltyshev I have not seen that when testing but I will take a look later today!

What there a stack trace with that deeper that the JSON issue?

@meltyshev
Copy link
Member

My bad, this error is just on the client, because the response did not come in JSON format and the fetch failed to parse. In the real response I see the following:

TypeError: Cannot read properties of null (reading 'length')
    at getRemoteAddress (/Users/ghostly/Projects/planka/server/utils/remoteAddress.js:18:44)
    at Object.fn (/Users/ghostly/Projects/planka/server/api/controllers/access-tokens/create.js:56:50)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

But even if it's commented out, everything works, but there are no logs in the console and in the logfile too.

@StevenJCorreia
Copy link
Contributor Author

StevenJCorreia commented Aug 22, 2022

@meltyshev

Regarding the length issue, I see now that I was not properly null-checking that object. Will fix that later today!

As for neither log sink working as intended:
Looks like the issue here is utils/logger::logLevel.
I assumed that Winston would accept integers as opposed to the strings that are noted in all of their examples. This was my attempt to enhance readability of the code..

# const logLevel = process.env.NODE_ENV === 'production' ? logLevels.info : logLevels.debug;
const logLevel = process.env.NODE_ENV === 'production' ? 'info' : 'debug';

Two things could work here:

  1. We could revert back to using string-literals as shown in above code block, or..
  2. We could convert logLevels to an array of strings so we still retain the priority levels and still have someone a bit more readable: E.x., logLevels[2] # 'info'

Let me know what you think works best here.

@meltyshev
Copy link
Member

I would prefer just the string-literals. Now with these changes, it works perfectly 👍

README.md Outdated Show resolved Hide resolved
@StevenJCorreia
Copy link
Contributor Author

@meltyshev Ready for re-review!

@meltyshev meltyshev merged commit 6429e22 into plankanban:master Aug 22, 2022
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.

None yet

2 participants