35
I remember having read somewhere (possibly in another Code Review answer) that for an e-mail address, the simplest and most effective validation you can do is to make sure it contains an @. Making it more restrictive than that can often be a risk of invalidating some valid e-mails. You'd be surprised at some examples of valid e-mail addresses.
As an ...
31
"@"@example.com and "\ "@example.com both fail, but they are valid.
" "@example.com passes, but it is, in fact, invalid.*
You probably missed the idea to confirm your knowledge with the relevant RFCs, as a conforming implementation should abide by the rules described therein. While Wikipedia is quite reliable nowadays, it is by no means a normative source....
27
Your script is vulnerable to a header-splitting attack. Due to the poor design of PHP's mail() function, it is actually quite easy to introduce that kind of security hole.
In summary, if…
any part of the mail headers consists of user-supplied input,
and you didn't make any effort to prohibit newlines or escape that input,
then you will have a program ...
23
I personally find it hard to fault your code.
Actually I'm quite surprised about the absence of code.
Other than a few PEP8 errors there are three changes that I would recommend.
You remove both \\ and \" from your quotes, but you do it in an overly verbose way:
stripped = quote.replace('\\\\', '').replace('\\"', '"').strip('".')
Instead you can use re....
22
Said this in chat already, but @ succeeds even although it is not a valid email address. You should require at least 1 character in the local part and 1 character in the domain.
22
Define some constants
I'd make some constants and put them at the top of the program because if your program becomes bigger, it'll be harder to change their values:
EMAIL_TIME = '06:30'
SLEEP_TIME = 1
URL = 'https://api.coindesk.com/v1/bpi/currentprice.json'
Never store your credentials in clear-text
Here, you replaced your actual password with a ...
20
As @Simon pointed out, your regular expression might consider some valid addresses as invalid.
Per this source which I've found somewhere on this Stack Overflow page, this would be RFC5322-compliant:
(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*
| "(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]
| \\[\x01-\x09\...
19
Your mailer is vulnerable to a header-splitting attack! (Also known as a CRLF-injection attack.)
Basically, if one of the fields contains a CRLF, the attacker can insert any arbitrary header into the generated message. For example, if the email parameter is
victim1@example.com%0d%0aCc:%20victim2@example.net
… then you will compose a message with headers
...
19
I'm not a php expert, but from reading up on the documentation it appears that it may be possible to insert additional from/to addresses using the "Additional Headers" field on the mail() function.
Since you're using a regular string concatenation to insert your POST variable into your additional headers field, that is very well possibly the vector for ...
17
There are no security vulnerabilities per se present, as far as I can see (however, 200_success did find one rather important one). But if you're security-conscious, you would probably want to practice defensive programming, which implies careful input validation and eschewing potentially buggy constructs.
One of the potentially buggy constructs in many ...
17
I disagree with rolfl's assertion that, "There is no practical way to validate an e-mail address by regex alone."
He is correct—as illustrated by the somewhat infamous SO answer he linked to—that it's impractical to validate any RFC-5322-compliant email addresses because, to quote the HTML5 spec:
RFC-5322 … defines a syntax for e-mail addresses that is ...
17
First, heed what Jeroen Vannevel said; it will save you much grief.
Of all the assumptions you've made here, only one is correct: there
has to be an @ in the email address. Otherwise: it can have multiple
dots, it can have no dots, it can have no TLD, it can have no domain,
it can have IP addresses as domain, it can have quotes, it can have
...
16
You seems to build your module to contain only validate as "public" function. You may want to enforce that by declaring __all__ = ['validate', 'InvalidEmail']. It will affect the way that pydoc and the help builtin display help on your module (they will show only the module docstring, the exception and the validate function) as well as how from ...
answered Jan 22 '16 at 13:02
301_Moved_Permanently
27.1k3 gold badges36 silver badges89 bronze badges
16
It seems to me that running a specific command at a specific time each day is more the job of a cron daemon than of said command. As such, I would simplify the executable part of the script to:
if __name__ == '__main__':
send_email()
and add an entry in your cron configuration that read something like
30 6 * * * /usr/bin/env python /path/to/your/...
answered Jan 4 '18 at 10:03
301_Moved_Permanently
27.1k3 gold badges36 silver badges89 bronze badges
15
Couple of remarks:
IsSent() smells like a property, not a method. It should be more descriptive as well: what object are you calling this on? It seems to indicate Email.IsSent but that's not what you do: your description makes it sound more like EmailManager.IsDailyEmailSent.
Dispose your SmtpClient instance.
Don't set isEmailSend to true before you ...
14
Email validation aside...
Switching between GUI input and console output
You take your input via a JOptionPane, but the output of your validation is displayed via System.out. It will be much better UX-wise to show the result as another dialog box too.
Braces
Please use braces, it's not that hard. One extra line here or there and your conditions/loops ...
13
It was suggested that I move this to an answer instead of a comment:
Since you're using .NET, why not just use System.Net.Mail.MailAddress? It already performs validation in its constructor - it will throw a FormatException if the address is invalid.
This is particularly useful if you intend on using the address to actually send email, since you'll likely ...
13
I would change the data organization to be the opposite way, to have a dict mapping to the two week types containing a dict with a mapping from the weekday to the lessons:
timetable = {"A": {"Monday": ["English", "Maths", ...],
"Tuesday": ["Maths", "Computing", ...],
...},
"B": {"Monday": ["History", ...],
...
12
LINQ isn't a silver bullet. It stands for Language-INtegrated-Query, which allows querying objects.
Querying objects isn't something that's supposed to have side-effects. However this is precisely what you loop's body is doing.
Therefore, refactoring it to use LINQ, if at all possible, would make it much less readable than the foreach loop you have here.
...
12
Broken HTML
Browsers are forgiving. I see a fairly nice looking page,
but the truth is, this HTML is seriously broken.
<table cellpadding="0" cellspacing="0" width="600" bgcolor="#FFFDF2">
<tr>
<td style="padding: 5px 0px 5px 0px;" bgcolor="#FFFDF2">
<tr>
<td align="center"><a href=""...
12
Let's start with the obvious: this code doesn't run. You're missing ans = starter() so that further (el)if ans.lower() == ... doesn't miserably fail with a NameError.
Likely, you define q() but never use it.
And you also appears to have other useless stuff floating around: why use both textblob and indicoio to perform sentiment analysis? You also seem to ...
answered Jan 28 '16 at 22:09
301_Moved_Permanently
27.1k3 gold badges36 silver badges89 bronze badges
11
Some thoughts about the code itself.
In general method names recommended as Pascal Case so sendEmail should ideally be named SendEmail
I might consider trying to make the class a bit more configurable so that it could be extended in more ways without being modified. For example if you wanted to send a text email, or you wanted to send an email using a ...
11
Argument handling
$defaultmail and $defaultip are poorly named — there is a naming inconsistency:
$host ||= $defaultip;
…
$mailto ||= $defaultmail;
I would also advise against introducing $defaultmail and $defaultip as variables at all. Either just write them in directly:
my $host = (shift @ARGV) || '8.8.8.8'; # Google's nameserver
my $...
11
I'll proceed top to bottom.
mail_error
Which would you rather read: x.body = e.message or mail.body = exception.message? Don't try to save a few characters on naming — your fellow programmers will like you better. In particular, x has a connotation of being a floating point number, which this isn't.
scanning_all_servers and server_status
Net::SSH#start ...
11
The code that you've written is generally really good, but as you seem to have found parsing non-trivial strings starts to get kind of complicated, and has all sorts of room for nasty edge cases.
Your remove_comments method appears not to account for nested comments, which are explicitly allowed by the RFC.
As expected, remove_comments("Hello (new) world") ...
10
Normally, you don't catch Exceptions to throw them again, it doesn't make sense, let them rise up, don't catch them here, I don't think it is your intention to stop the exceptions here, so don't.
Let the Exceptions bubble up.
If you are checking in a database to see if it is sent already you could save to the database with a dateSent column or something ...
10
Here's some changes I would make.
First thing I would do is make most of this module level variables, initialized in your Application_startup method, and taking advantage of WithEvents to setup the listener.
Option Explicit
Private nameSpace As Outlook.nameSpace
Private inbox As Outlook.Folder
Private destFolder As Outlook.Folder
Private WithEvents items ...
10
What you really want is a robust argument parsing. This calls for argparse:
import argparse
import os.path
def is_file(f):
if os.path.isfile(f):
return f
raise OSError("{} does not exist".format(f))
parser = argparse.ArgumentParser(description="SMTP user checker")
parser.add_argument('server', help="IP address of server")
parser....
10
The goals of this will be to make the code more readable and maintainable.
Don't put templates and credentials in your code.
Most of the class-level declarations of MessageUser are setting up stuff. You should store your configurable parameters in a configuration file. Check out the Python documentation for more info. This ought to be enough. Note it does ...
10
The code is straightforward and reads well, but:
string concatenation is not very efficient, especially in a loop. You'd be better off using str.join on an iterable;
encoding the mailto: part yourself impairs readability and maintenance, if only you had a function to do it for you. Oh wait...
The comment at the beginning would be better as a module ...
answered Oct 7 '18 at 15:00
301_Moved_Permanently
27.1k3 gold badges36 silver badges89 bronze badges
Only top voted, non community-wiki answers of a minimum length are eligible
